From 99b662c6b7c0b2335887eb6298e7c5bc598b93b1 Mon Sep 17 00:00:00 2001 From: Ryan Arakawa Date: Mon, 16 Feb 2026 16:14:39 -0600 Subject: [PATCH 01/10] add partnership_id to webhook_urls - add migration to make account_id OR partnership_id required, you can use either, but can and must use at least one - add PARTNERSHIP_EVENTS constant to constrain webhook firing to just template events --- app/models/webhook_url.rb | 41 +++++--- ...1605_add_partnership_id_to_webhook_urls.rb | 14 +++ db/schema.rb | 17 ++-- spec/models/webhook_url_spec.rb | 96 +++++++++++++++++++ 4 files changed, 150 insertions(+), 18 deletions(-) create mode 100644 db/migrate/20260206171605_add_partnership_id_to_webhook_urls.rb create mode 100644 spec/models/webhook_url_spec.rb diff --git a/app/models/webhook_url.rb b/app/models/webhook_url.rb index 6f3ec5ae7..f08c7a640 100644 --- a/app/models/webhook_url.rb +++ b/app/models/webhook_url.rb @@ -4,23 +4,26 @@ # # Table name: webhook_urls # -# id :bigint not null, primary key -# events :text not null -# secret :text not null -# sha1 :string not null -# url :text not null -# created_at :datetime not null -# updated_at :datetime not null -# account_id :integer not null +# id :bigint not null, primary key +# events :text not null +# secret :text not null +# sha1 :string not null +# url :text not null +# created_at :datetime not null +# updated_at :datetime not null +# account_id :integer +# partnership_id :bigint # # Indexes # -# index_webhook_urls_on_account_id (account_id) -# index_webhook_urls_on_sha1 (sha1) +# index_webhook_urls_on_account_id (account_id) +# index_webhook_urls_on_partnership_id (partnership_id) +# index_webhook_urls_on_sha1 (sha1) # # Foreign Keys # # fk_rails_... (account_id => accounts.id) +# fk_rails_... (partnership_id => partnerships.id) # class WebhookUrl < ApplicationRecord EVENTS = %w[ @@ -36,7 +39,14 @@ class WebhookUrl < ApplicationRecord template.updated ].freeze - belongs_to :account + # Partnership webhooks can only use template events since partnerships don't have submissions/submitters + PARTNERSHIP_EVENTS = %w[ + template.created + template.updated + ].freeze + + belongs_to :account, optional: true + belongs_to :partnership, optional: true attribute :events, :string, default: -> { %w[form.viewed form.started form.completed form.declined] } attribute :secret, :string, default: -> { {} } @@ -45,10 +55,19 @@ class WebhookUrl < ApplicationRecord serialize :secret, coder: JSON before_validation :set_sha1 + validate :validate_owner_presence encrypts :url, :secret def set_sha1 self.sha1 = Digest::SHA1.hexdigest(url) end + + private + + def validate_owner_presence + return if account_id.present? ^ partnership_id.present? + + errors.add(:base, 'Must have either account_id or partnership_id, but not both') + end end diff --git a/db/migrate/20260206171605_add_partnership_id_to_webhook_urls.rb b/db/migrate/20260206171605_add_partnership_id_to_webhook_urls.rb new file mode 100644 index 000000000..53a358a21 --- /dev/null +++ b/db/migrate/20260206171605_add_partnership_id_to_webhook_urls.rb @@ -0,0 +1,14 @@ +class AddPartnershipIdToWebhookUrls < ActiveRecord::Migration[8.0] + def change + # Make account_id nullable since webhooks can now belong to either account or partnership + change_column_null :webhook_urls, :account_id, true + + # Add partnership_id as optional reference + add_reference :webhook_urls, :partnership, null: true, foreign_key: true + + # Add check constraint to ensure exactly one of account_id or partnership_id is set + add_check_constraint :webhook_urls, + '(account_id IS NOT NULL AND partnership_id IS NULL) OR (account_id IS NULL AND partnership_id IS NOT NULL)', + name: 'webhook_urls_owner_check' + end +end diff --git a/db/schema.rb b/db/schema.rb index b1e01cf03..f06135eb2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2026_01_21_191632) do +ActiveRecord::Schema[8.0].define(version: 2026_02_06_171605) do # These are extensions that must be enabled in order to support this database enable_extension "btree_gin" enable_extension "pg_catalog.plpgsql" @@ -181,7 +181,7 @@ t.datetime "created_at", null: false t.index ["account_id", "event_datetime"], name: "index_email_events_on_account_id_and_event_datetime" t.index ["email"], name: "index_email_events_on_email" - t.index ["email"], name: "index_email_events_on_email_event_types", where: "((event_type)::text = ANY (ARRAY[('bounce'::character varying)::text, ('soft_bounce'::character varying)::text, ('complaint'::character varying)::text, ('soft_complaint'::character varying)::text]))" + t.index ["email"], name: "index_email_events_on_email_event_types", where: "((event_type)::text = ANY ((ARRAY['bounce'::character varying, 'soft_bounce'::character varying, 'complaint'::character varying, 'soft_complaint'::character varying])::text[]))" t.index ["emailable_type", "emailable_id"], name: "index_email_events_on_emailable" t.index ["message_id"], name: "index_email_events_on_message_id" end @@ -290,11 +290,10 @@ t.tsvector "tsvector", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.index ["account_id"], name: "index_search_entries_on_account_id" + t.index ["account_id", "tsvector"], name: "index_search_entries_on_account_id_tsvector_submission", where: "((record_type)::text = 'Submission'::text)", using: :gin + t.index ["account_id", "tsvector"], name: "index_search_entries_on_account_id_tsvector_submitter", where: "((record_type)::text = 'Submitter'::text)", using: :gin + t.index ["account_id", "tsvector"], name: "index_search_entries_on_account_id_tsvector_template", where: "((record_type)::text = 'Template'::text)", using: :gin t.index ["record_id", "record_type"], name: "index_search_entries_on_record_id_and_record_type", unique: true - t.index ["tsvector"], name: "index_search_entries_on_account_id_tsvector_submission", where: "((record_type)::text = 'Submission'::text)", using: :gin - t.index ["tsvector"], name: "index_search_entries_on_account_id_tsvector_submitter", where: "((record_type)::text = 'Submitter'::text)", using: :gin - t.index ["tsvector"], name: "index_search_entries_on_account_id_tsvector_template", where: "((record_type)::text = 'Template'::text)", using: :gin end create_table "submission_events", force: :cascade do |t| @@ -470,15 +469,18 @@ end create_table "webhook_urls", force: :cascade do |t| - t.integer "account_id", null: false + t.integer "account_id" t.text "url", null: false t.text "events", null: false t.string "sha1", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false t.text "secret", null: false + t.bigint "partnership_id" t.index ["account_id"], name: "index_webhook_urls_on_account_id" + t.index ["partnership_id"], name: "index_webhook_urls_on_partnership_id" t.index ["sha1"], name: "index_webhook_urls_on_sha1" + t.check_constraint "account_id IS NOT NULL AND partnership_id IS NULL OR account_id IS NULL AND partnership_id IS NOT NULL", name: "webhook_urls_owner_check" end add_foreign_key "access_tokens", "users" @@ -516,4 +518,5 @@ add_foreign_key "user_configs", "users" add_foreign_key "users", "accounts" add_foreign_key "webhook_urls", "accounts" + add_foreign_key "webhook_urls", "partnerships" end diff --git a/spec/models/webhook_url_spec.rb b/spec/models/webhook_url_spec.rb new file mode 100644 index 000000000..f735ab1d4 --- /dev/null +++ b/spec/models/webhook_url_spec.rb @@ -0,0 +1,96 @@ +# frozen_string_literal: true + +# == Schema Information +# +# Table name: webhook_urls +# +# id :bigint not null, primary key +# events :text not null +# secret :text not null +# sha1 :string not null +# url :text not null +# created_at :datetime not null +# updated_at :datetime not null +# account_id :integer +# partnership_id :bigint +# +# Indexes +# +# index_webhook_urls_on_account_id (account_id) +# index_webhook_urls_on_partnership_id (partnership_id) +# index_webhook_urls_on_sha1 (sha1) +# +# Foreign Keys +# +# fk_rails_... (account_id => accounts.id) +# fk_rails_... (partnership_id => partnerships.id) +# +describe WebhookUrl do + describe 'validations' do + context 'owner presence' do + it 'is valid with account_id and no partnership_id' do + webhook = build(:webhook_url, account: create(:account), partnership: nil) + expect(webhook).to be_valid + end + + it 'is valid with partnership_id and no account_id' do + partnership = nil + # Disable webhook creation callback for this test + allow_any_instance_of(Partnership).to receive(:create_careerplug_webhook) + partnership = create(:partnership) + webhook = build(:webhook_url, + account: nil, + partnership: partnership, + events: WebhookUrl::PARTNERSHIP_EVENTS) + expect(webhook).to be_valid + end + + it 'is invalid with both account_id and partnership_id' do + webhook = build(:webhook_url, account: create(:account), partnership: create(:partnership)) + expect(webhook).not_to be_valid + expect(webhook.errors[:base]).to include('Must have either account_id or partnership_id, but not both') + end + + it 'is invalid with neither account_id nor partnership_id' do + webhook = build(:webhook_url, account: nil, partnership: nil) + expect(webhook).not_to be_valid + expect(webhook.errors[:base]).to include('Must have either account_id or partnership_id, but not both') + end + end + + context 'partnership events constraint' do + it 'only includes template.* events in PARTNERSHIP_EVENTS' do + WebhookUrl::PARTNERSHIP_EVENTS.each do |event| + expect(event).to start_with('template.'), + "PARTNERSHIP_EVENTS should only contain 'template.*' events, found: #{event}. " \ + 'Partnerships do not have submissions or submitters.' + end + end + + it 'PARTNERSHIP_EVENTS is a subset of EVENTS' do + expect(WebhookUrl::PARTNERSHIP_EVENTS).to all(be_in(WebhookUrl::EVENTS)) + end + end + end + + describe 'callbacks' do + describe '#set_sha1' do + it 'sets sha1 based on url' do + webhook = build(:webhook_url, url: 'https://example.com/webhook') + webhook.valid? + expect(webhook.sha1).to eq(Digest::SHA1.hexdigest('https://example.com/webhook')) + end + + it 'updates sha1 when url changes' do + webhook = create(:webhook_url, url: 'https://example.com/webhook') + original_sha1 = webhook.sha1 + + webhook.url = 'https://example.com/new-webhook' + webhook.valid? + + expect(webhook.sha1).not_to eq(original_sha1) + expect(webhook.sha1).to eq(Digest::SHA1.hexdigest('https://example.com/new-webhook')) + end + end + end +end From b7a78be184e5c38e144d1be18ca8055394fef54b Mon Sep 17 00:00:00 2001 From: Ryan Arakawa Date: Mon, 16 Feb 2026 16:18:47 -0600 Subject: [PATCH 02/10] extract duplicated webhook retry logic for webhook jobs - 11 webhook jobs all used the same retry logic (except one file that had 12 max retries instead of 10. - remove and replace duplicated code - add retry logic for partnership templates --- ...m_changes_requested_webhook_request_job.rb | 19 +- ...send_form_completed_webhook_request_job.rb | 17 +- .../send_form_declined_webhook_request_job.rb | 19 +- .../send_form_started_webhook_request_job.rb | 19 +- .../send_form_viewed_webhook_request_job.rb | 19 +- ...submission_archived_webhook_request_job.rb | 19 +- ...ubmission_completed_webhook_request_job.rb | 17 +- ..._submission_created_webhook_request_job.rb | 19 +- ..._submission_expired_webhook_request_job.rb | 19 +- ...nd_template_created_webhook_request_job.rb | 17 +- ...nd_template_updated_webhook_request_job.rb | 17 +- lib/webhook_retry_logic.rb | 37 +++ ...mplate_created_webhook_request_job_spec.rb | 62 +++++ ...mplate_updated_webhook_request_job_spec.rb | 62 +++++ spec/lib/webhook_retry_logic_spec.rb | 216 ++++++++++++++++++ 15 files changed, 463 insertions(+), 115 deletions(-) create mode 100644 lib/webhook_retry_logic.rb create mode 100644 spec/lib/webhook_retry_logic_spec.rb diff --git a/app/jobs/send_form_changes_requested_webhook_request_job.rb b/app/jobs/send_form_changes_requested_webhook_request_job.rb index 79bf345eb..950d9db7a 100644 --- a/app/jobs/send_form_changes_requested_webhook_request_job.rb +++ b/app/jobs/send_form_changes_requested_webhook_request_job.rb @@ -5,8 +5,6 @@ class SendFormChangesRequestedWebhookRequestJob sidekiq_options queue: :webhooks - MAX_ATTEMPTS = 10 - def perform(params = {}) submitter = Submitter.find(params['submitter_id']) webhook_url = WebhookUrl.find(params['webhook_url_id']) @@ -20,14 +18,13 @@ def perform(params = {}) resp = SendWebhookRequest.call(webhook_url, event_type: 'form.changes_requested', data: Submitters::SerializeForWebhook.call(submitter)) - if (resp.nil? || resp.status.to_i >= 400) && attempt <= MAX_ATTEMPTS && - (!Docuseal.multitenant? || submitter.account.account_configs.exists?(key: :plan)) - SendFormChangesRequestedWebhookRequestJob.perform_in((2**attempt).minutes, { - 'submitter_id' => submitter.id, - 'webhook_url_id' => webhook_url.id, - 'attempt' => attempt + 1, - 'last_status' => resp&.status.to_i - }) - end + return unless WebhookRetryLogic.should_retry?(response: resp, attempt: attempt, record: submitter) + + SendFormChangesRequestedWebhookRequestJob.perform_in((2**attempt).minutes, { + 'submitter_id' => submitter.id, + 'webhook_url_id' => webhook_url.id, + 'attempt' => attempt + 1, + 'last_status' => resp&.status.to_i + }) end end diff --git a/app/jobs/send_form_completed_webhook_request_job.rb b/app/jobs/send_form_completed_webhook_request_job.rb index 6897746b0..e4a1340d6 100644 --- a/app/jobs/send_form_completed_webhook_request_job.rb +++ b/app/jobs/send_form_completed_webhook_request_job.rb @@ -5,8 +5,6 @@ class SendFormCompletedWebhookRequestJob sidekiq_options queue: :webhooks - MAX_ATTEMPTS = 12 - def perform(params = {}) submitter = Submitter.find(params['submitter_id']) webhook_url = WebhookUrl.find(params['webhook_url_id']) @@ -28,14 +26,13 @@ def perform(params = {}) resp = SendWebhookRequest.call(webhook_url, event_type: 'form.completed', data: webhook_data) - if (resp.nil? || resp.status.to_i >= 400) && attempt <= MAX_ATTEMPTS && - (!Docuseal.multitenant? || submitter.account.account_configs.exists?(key: :plan)) - SendFormCompletedWebhookRequestJob.perform_in((2**attempt).minutes, { - **params, - 'attempt' => attempt + 1, - 'last_status' => resp&.status.to_i - }) - end + return unless WebhookRetryLogic.should_retry?(response: resp, attempt: attempt, record: submitter) + + SendFormCompletedWebhookRequestJob.perform_in((2**attempt).minutes, { + **params, + 'attempt' => attempt + 1, + 'last_status' => resp&.status.to_i + }) end private diff --git a/app/jobs/send_form_declined_webhook_request_job.rb b/app/jobs/send_form_declined_webhook_request_job.rb index 1c7a1e32c..e571c8fd1 100644 --- a/app/jobs/send_form_declined_webhook_request_job.rb +++ b/app/jobs/send_form_declined_webhook_request_job.rb @@ -5,8 +5,6 @@ class SendFormDeclinedWebhookRequestJob sidekiq_options queue: :webhooks - MAX_ATTEMPTS = 10 - def perform(params = {}) submitter = Submitter.find(params['submitter_id']) webhook_url = WebhookUrl.find(params['webhook_url_id']) @@ -20,14 +18,13 @@ def perform(params = {}) resp = SendWebhookRequest.call(webhook_url, event_type: 'form.declined', data: Submitters::SerializeForWebhook.call(submitter)) - if (resp.nil? || resp.status.to_i >= 400) && attempt <= MAX_ATTEMPTS && - (!Docuseal.multitenant? || submitter.account.account_configs.exists?(key: :plan)) - SendFormDeclinedWebhookRequestJob.perform_in((2**attempt).minutes, { - 'submitter_id' => submitter.id, - 'webhook_url_id' => webhook_url.id, - 'attempt' => attempt + 1, - 'last_status' => resp&.status.to_i - }) - end + return unless WebhookRetryLogic.should_retry?(response: resp, attempt: attempt, record: submitter) + + SendFormDeclinedWebhookRequestJob.perform_in((2**attempt).minutes, { + 'submitter_id' => submitter.id, + 'webhook_url_id' => webhook_url.id, + 'attempt' => attempt + 1, + 'last_status' => resp&.status.to_i + }) end end diff --git a/app/jobs/send_form_started_webhook_request_job.rb b/app/jobs/send_form_started_webhook_request_job.rb index 44510f463..e3b1d1384 100644 --- a/app/jobs/send_form_started_webhook_request_job.rb +++ b/app/jobs/send_form_started_webhook_request_job.rb @@ -5,8 +5,6 @@ class SendFormStartedWebhookRequestJob sidekiq_options queue: :webhooks - MAX_ATTEMPTS = 10 - def perform(params = {}) submitter = Submitter.find(params['submitter_id']) webhook_url = WebhookUrl.find(params['webhook_url_id']) @@ -20,14 +18,13 @@ def perform(params = {}) resp = SendWebhookRequest.call(webhook_url, event_type: 'form.started', data: Submitters::SerializeForWebhook.call(submitter)) - if (resp.nil? || resp.status.to_i >= 400) && attempt <= MAX_ATTEMPTS && - (!Docuseal.multitenant? || submitter.account.account_configs.exists?(key: :plan)) - SendFormStartedWebhookRequestJob.perform_in((2**attempt).minutes, { - 'submitter_id' => submitter.id, - 'webhook_url_id' => webhook_url.id, - 'attempt' => attempt + 1, - 'last_status' => resp&.status.to_i - }) - end + return unless WebhookRetryLogic.should_retry?(response: resp, attempt: attempt, record: submitter) + + SendFormStartedWebhookRequestJob.perform_in((2**attempt).minutes, { + 'submitter_id' => submitter.id, + 'webhook_url_id' => webhook_url.id, + 'attempt' => attempt + 1, + 'last_status' => resp&.status.to_i + }) end end diff --git a/app/jobs/send_form_viewed_webhook_request_job.rb b/app/jobs/send_form_viewed_webhook_request_job.rb index 162743e43..5218461d3 100644 --- a/app/jobs/send_form_viewed_webhook_request_job.rb +++ b/app/jobs/send_form_viewed_webhook_request_job.rb @@ -5,8 +5,6 @@ class SendFormViewedWebhookRequestJob sidekiq_options queue: :webhooks - MAX_ATTEMPTS = 10 - def perform(params = {}) submitter = Submitter.find(params['submitter_id']) webhook_url = WebhookUrl.find(params['webhook_url_id']) @@ -20,14 +18,13 @@ def perform(params = {}) resp = SendWebhookRequest.call(webhook_url, event_type: 'form.viewed', data: Submitters::SerializeForWebhook.call(submitter)) - if (resp.nil? || resp.status.to_i >= 400) && attempt <= MAX_ATTEMPTS && - (!Docuseal.multitenant? || submitter.account.account_configs.exists?(key: :plan)) - SendFormViewedWebhookRequestJob.perform_in((2**attempt).minutes, { - 'submitter_id' => submitter.id, - 'webhook_url_id' => webhook_url.id, - 'attempt' => attempt + 1, - 'last_status' => resp&.status.to_i - }) - end + return unless WebhookRetryLogic.should_retry?(response: resp, attempt: attempt, record: submitter) + + SendFormViewedWebhookRequestJob.perform_in((2**attempt).minutes, { + 'submitter_id' => submitter.id, + 'webhook_url_id' => webhook_url.id, + 'attempt' => attempt + 1, + 'last_status' => resp&.status.to_i + }) end end diff --git a/app/jobs/send_submission_archived_webhook_request_job.rb b/app/jobs/send_submission_archived_webhook_request_job.rb index 82fc271f4..56f3c0be0 100644 --- a/app/jobs/send_submission_archived_webhook_request_job.rb +++ b/app/jobs/send_submission_archived_webhook_request_job.rb @@ -5,8 +5,6 @@ class SendSubmissionArchivedWebhookRequestJob sidekiq_options queue: :webhooks - MAX_ATTEMPTS = 10 - def perform(params = {}) submission = Submission.find(params['submission_id']) webhook_url = WebhookUrl.find(params['webhook_url_id']) @@ -18,14 +16,13 @@ def perform(params = {}) resp = SendWebhookRequest.call(webhook_url, event_type: 'submission.archived', data: submission.as_json(only: %i[id archived_at])) - if (resp.nil? || resp.status.to_i >= 400) && attempt <= MAX_ATTEMPTS && - (!Docuseal.multitenant? || submission.account.account_configs.exists?(key: :plan)) - SendSubmissionArchivedWebhookRequestJob.perform_in((2**attempt).minutes, { - 'submission_id' => submission.id, - 'webhook_url_id' => webhook_url.id, - 'attempt' => attempt + 1, - 'last_status' => resp&.status.to_i - }) - end + return unless WebhookRetryLogic.should_retry?(response: resp, attempt: attempt, record: submission) + + SendSubmissionArchivedWebhookRequestJob.perform_in((2**attempt).minutes, { + 'submission_id' => submission.id, + 'webhook_url_id' => webhook_url.id, + 'attempt' => attempt + 1, + 'last_status' => resp&.status.to_i + }) end end diff --git a/app/jobs/send_submission_completed_webhook_request_job.rb b/app/jobs/send_submission_completed_webhook_request_job.rb index 375bfa759..847933601 100644 --- a/app/jobs/send_submission_completed_webhook_request_job.rb +++ b/app/jobs/send_submission_completed_webhook_request_job.rb @@ -5,8 +5,6 @@ class SendSubmissionCompletedWebhookRequestJob sidekiq_options queue: :webhooks - MAX_ATTEMPTS = 10 - def perform(params = {}) submission = Submission.find(params['submission_id']) webhook_url = WebhookUrl.find(params['webhook_url_id']) @@ -18,13 +16,12 @@ def perform(params = {}) resp = SendWebhookRequest.call(webhook_url, event_type: 'submission.completed', data: Submissions::SerializeForApi.call(submission)) - if (resp.nil? || resp.status.to_i >= 400) && attempt <= MAX_ATTEMPTS && - (!Docuseal.multitenant? || submission.account.account_configs.exists?(key: :plan)) - SendSubmissionCompletedWebhookRequestJob.perform_in((2**attempt).minutes, { - **params, - 'attempt' => attempt + 1, - 'last_status' => resp&.status.to_i - }) - end + return unless WebhookRetryLogic.should_retry?(response: resp, attempt: attempt, record: submission) + + SendSubmissionCompletedWebhookRequestJob.perform_in((2**attempt).minutes, { + **params, + 'attempt' => attempt + 1, + 'last_status' => resp&.status.to_i + }) end end diff --git a/app/jobs/send_submission_created_webhook_request_job.rb b/app/jobs/send_submission_created_webhook_request_job.rb index d798e76a8..2ae47e2e1 100644 --- a/app/jobs/send_submission_created_webhook_request_job.rb +++ b/app/jobs/send_submission_created_webhook_request_job.rb @@ -5,8 +5,6 @@ class SendSubmissionCreatedWebhookRequestJob sidekiq_options queue: :webhooks - MAX_ATTEMPTS = 10 - def perform(params = {}) submission = Submission.find(params['submission_id']) webhook_url = WebhookUrl.find(params['webhook_url_id']) @@ -18,14 +16,13 @@ def perform(params = {}) resp = SendWebhookRequest.call(webhook_url, event_type: 'submission.created', data: Submissions::SerializeForApi.call(submission)) - if (resp.nil? || resp.status.to_i >= 400) && attempt <= MAX_ATTEMPTS && - (!Docuseal.multitenant? || submission.account.account_configs.exists?(key: :plan)) - SendSubmissionCreatedWebhookRequestJob.perform_in((2**attempt).minutes, { - 'submission_id' => submission.id, - 'webhook_url_id' => webhook_url.id, - 'attempt' => attempt + 1, - 'last_status' => resp&.status.to_i - }) - end + return unless WebhookRetryLogic.should_retry?(response: resp, attempt: attempt, record: submission) + + SendSubmissionCreatedWebhookRequestJob.perform_in((2**attempt).minutes, { + 'submission_id' => submission.id, + 'webhook_url_id' => webhook_url.id, + 'attempt' => attempt + 1, + 'last_status' => resp&.status.to_i + }) end end diff --git a/app/jobs/send_submission_expired_webhook_request_job.rb b/app/jobs/send_submission_expired_webhook_request_job.rb index c2a5691b8..c361ba381 100644 --- a/app/jobs/send_submission_expired_webhook_request_job.rb +++ b/app/jobs/send_submission_expired_webhook_request_job.rb @@ -5,8 +5,6 @@ class SendSubmissionExpiredWebhookRequestJob sidekiq_options queue: :webhooks - MAX_ATTEMPTS = 10 - def perform(params = {}) submission = Submission.find(params['submission_id']) webhook_url = WebhookUrl.find(params['webhook_url_id']) @@ -18,14 +16,13 @@ def perform(params = {}) resp = SendWebhookRequest.call(webhook_url, event_type: 'submission.expired', data: Submissions::SerializeForApi.call(submission)) - if (resp.nil? || resp.status.to_i >= 400) && attempt <= MAX_ATTEMPTS && - (!Docuseal.multitenant? || submission.account.account_configs.exists?(key: :plan)) - SendSubmissionExpiredWebhookRequestJob.perform_in((2**attempt).minutes, { - 'submission_id' => submission.id, - 'webhook_url_id' => webhook_url.id, - 'attempt' => attempt + 1, - 'last_status' => resp&.status.to_i - }) - end + return unless WebhookRetryLogic.should_retry?(response: resp, attempt: attempt, record: submission) + + SendSubmissionExpiredWebhookRequestJob.perform_in((2**attempt).minutes, { + 'submission_id' => submission.id, + 'webhook_url_id' => webhook_url.id, + 'attempt' => attempt + 1, + 'last_status' => resp&.status.to_i + }) end end diff --git a/app/jobs/send_template_created_webhook_request_job.rb b/app/jobs/send_template_created_webhook_request_job.rb index 353ecb6d5..7fa493792 100644 --- a/app/jobs/send_template_created_webhook_request_job.rb +++ b/app/jobs/send_template_created_webhook_request_job.rb @@ -18,14 +18,13 @@ def perform(params = {}) resp = SendWebhookRequest.call(webhook_url, event_type: 'template.created', data: Templates::SerializeForApi.call(template)) - if (resp.nil? || resp.status.to_i >= 400) && attempt <= MAX_ATTEMPTS && - (!Docuseal.multitenant? || template.account.account_configs.exists?(key: :plan)) - SendTemplateCreatedWebhookRequestJob.perform_in((2**attempt).minutes, { - 'template_id' => template.id, - 'webhook_url_id' => webhook_url.id, - 'attempt' => attempt + 1, - 'last_status' => resp&.status.to_i - }) - end + return unless WebhookRetryLogic.should_retry?(response: resp, attempt: attempt, record: template) + + SendTemplateCreatedWebhookRequestJob.perform_in((2**attempt).minutes, { + 'template_id' => template.id, + 'webhook_url_id' => webhook_url.id, + 'attempt' => attempt + 1, + 'last_status' => resp&.status.to_i + }) end end diff --git a/app/jobs/send_template_updated_webhook_request_job.rb b/app/jobs/send_template_updated_webhook_request_job.rb index 30623e15b..59d945a4a 100644 --- a/app/jobs/send_template_updated_webhook_request_job.rb +++ b/app/jobs/send_template_updated_webhook_request_job.rb @@ -18,14 +18,13 @@ def perform(params = {}) resp = SendWebhookRequest.call(webhook_url, event_type: 'template.updated', data: Templates::SerializeForApi.call(template)) - if (resp.nil? || resp.status.to_i >= 400) && attempt <= MAX_ATTEMPTS && - (!Docuseal.multitenant? || template.account.account_configs.exists?(key: :plan)) - SendTemplateUpdatedWebhookRequestJob.perform_in((2**attempt).minutes, { - 'template_id' => template.id, - 'webhook_url_id' => webhook_url.id, - 'attempt' => attempt + 1, - 'last_status' => resp&.status.to_i - }) - end + return unless WebhookRetryLogic.should_retry?(response: resp, attempt: attempt, record: template) + + SendTemplateUpdatedWebhookRequestJob.perform_in((2**attempt).minutes, { + 'template_id' => template.id, + 'webhook_url_id' => webhook_url.id, + 'attempt' => attempt + 1, + 'last_status' => resp&.status.to_i + }) end end diff --git a/lib/webhook_retry_logic.rb b/lib/webhook_retry_logic.rb new file mode 100644 index 000000000..b839b8593 --- /dev/null +++ b/lib/webhook_retry_logic.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +# Shared logic for determining if webhook requests should be retried +# Used across all Send*WebhookRequestJob classes +module WebhookRetryLogic + module_function + + MAX_ATTEMPTS = 10 + + # Determines if a failed webhook request should be retried + # + # @param response [HTTP::Response, nil] The HTTP response from the webhook request + # @param attempt [Integer] Current retry attempt number + # @param record [Template, Submission, Submitter] The record triggering the webhook + # @return [Boolean] true if the webhook should be retried + def should_retry?(response:, attempt:, record:) + return false unless response.nil? || response.status.to_i >= 400 + return false if attempt > MAX_ATTEMPTS + return true unless Docuseal.multitenant? + + eligible_for_retries?(record) + end + + # Checks if a record is eligible for webhook retries in multitenant mode + # @param record [Template, Submission, Submitter] The record to check + # @return [Boolean] true if eligible for retries + def eligible_for_retries?(record) + case record + when Template + record.partnership_id.present? || record.account&.account_configs&.exists?(key: :plan) + when Submission, Submitter + record.account.account_configs.exists?(key: :plan) + else + false + end + end +end diff --git a/spec/jobs/send_template_created_webhook_request_job_spec.rb b/spec/jobs/send_template_created_webhook_request_job_spec.rb index bb2b51c63..3ad3d779a 100644 --- a/spec/jobs/send_template_created_webhook_request_job_spec.rb +++ b/spec/jobs/send_template_created_webhook_request_job_spec.rb @@ -84,5 +84,67 @@ expect(WebMock).to have_requested(:post, webhook_url.url).once end + + context 'with partnership template' do + let(:partnership) { create(:partnership) } + let(:partnership_template) { create(:template, partnership: partnership, account: nil, author: user) } + let(:partnership_webhook) do + create(:webhook_url, + partnership: partnership, + account: nil, + events: ['template.created'], + url: 'https://partnership.example.com/webhook') + end + + before do + stub_request(:post, partnership_webhook.url).to_return(status: 200) + end + + it 'sends a webhook request for partnership template' do + described_class.new.perform('template_id' => partnership_template.id, + 'webhook_url_id' => partnership_webhook.id) + + expect(WebMock).to have_requested(:post, partnership_webhook.url).with( + body: { + 'event_type' => 'template.created', + 'timestamp' => /.*/, + 'data' => JSON.parse(Templates::SerializeForApi.call(partnership_template.reload).to_json) + }, + headers: { + 'Content-Type' => 'application/json', + 'User-Agent' => 'DocuSeal.com Webhook' + } + ).once + end + + it 'sends a webhook request with the partnership secret' do + partnership_webhook.update(secret: { 'X-Partnership-Secret' => 'partnership_secret' }) + described_class.new.perform('template_id' => partnership_template.id, + 'webhook_url_id' => partnership_webhook.id) + + expect(WebMock).to have_requested(:post, partnership_webhook.url).with( + headers: { + 'Content-Type' => 'application/json', + 'User-Agent' => 'DocuSeal.com Webhook', + 'X-Partnership-Secret' => 'partnership_secret' + } + ).once + end + + it 'retries on failure for partnership template' do + stub_request(:post, partnership_webhook.url).to_return(status: 500) + + expect do + described_class.new.perform('template_id' => partnership_template.id, + 'webhook_url_id' => partnership_webhook.id) + end.to change(described_class.jobs, :size).by(1) + + expect(WebMock).to have_requested(:post, partnership_webhook.url).once + + args = described_class.jobs.last['args'].first + expect(args['attempt']).to eq(1) + expect(args['last_status']).to eq(500) + end + end end end diff --git a/spec/jobs/send_template_updated_webhook_request_job_spec.rb b/spec/jobs/send_template_updated_webhook_request_job_spec.rb index 2edcd2800..aaf942fd6 100644 --- a/spec/jobs/send_template_updated_webhook_request_job_spec.rb +++ b/spec/jobs/send_template_updated_webhook_request_job_spec.rb @@ -84,5 +84,67 @@ expect(WebMock).to have_requested(:post, webhook_url.url).once end + + context 'with partnership template' do + let(:partnership) { create(:partnership) } + let(:partnership_template) { create(:template, partnership: partnership, account: nil, author: user) } + let(:partnership_webhook) do + create(:webhook_url, + partnership: partnership, + account: nil, + events: ['template.updated'], + url: 'https://partnership.example.com/webhook') + end + + before do + stub_request(:post, partnership_webhook.url).to_return(status: 200) + end + + it 'sends a webhook request for partnership template' do + described_class.new.perform('template_id' => partnership_template.id, + 'webhook_url_id' => partnership_webhook.id) + + expect(WebMock).to have_requested(:post, partnership_webhook.url).with( + body: { + 'event_type' => 'template.updated', + 'timestamp' => /.*/, + 'data' => JSON.parse(Templates::SerializeForApi.call(partnership_template.reload).to_json) + }, + headers: { + 'Content-Type' => 'application/json', + 'User-Agent' => 'DocuSeal.com Webhook' + } + ).once + end + + it 'sends a webhook request with the partnership secret' do + partnership_webhook.update(secret: { 'X-Partnership-Secret' => 'partnership_secret' }) + described_class.new.perform('template_id' => partnership_template.id, + 'webhook_url_id' => partnership_webhook.id) + + expect(WebMock).to have_requested(:post, partnership_webhook.url).with( + headers: { + 'Content-Type' => 'application/json', + 'User-Agent' => 'DocuSeal.com Webhook', + 'X-Partnership-Secret' => 'partnership_secret' + } + ).once + end + + it 'retries on failure for partnership template' do + stub_request(:post, partnership_webhook.url).to_return(status: 500) + + expect do + described_class.new.perform('template_id' => partnership_template.id, + 'webhook_url_id' => partnership_webhook.id) + end.to change(described_class.jobs, :size).by(1) + + expect(WebMock).to have_requested(:post, partnership_webhook.url).once + + args = described_class.jobs.last['args'].first + expect(args['attempt']).to eq(1) + expect(args['last_status']).to eq(500) + end + end end end diff --git a/spec/lib/webhook_retry_logic_spec.rb b/spec/lib/webhook_retry_logic_spec.rb new file mode 100644 index 000000000..71e2d9784 --- /dev/null +++ b/spec/lib/webhook_retry_logic_spec.rb @@ -0,0 +1,216 @@ +# frozen_string_literal: true + +RSpec.describe WebhookRetryLogic do + let(:account) { create(:account) } + let(:user) { create(:user, account: account) } + + describe '.should_retry?' do + context 'with successful response' do + let(:response) { double(status: 200) } + + it 'does not retry' do + template = create(:template, account: account, author: user) + result = described_class.should_retry?(response: response, attempt: 1, record: template) + expect(result).to be false + end + end + + context 'with nil response' do + let(:response) { nil } + + it 'retries within attempt limit' do + template = create(:template, account: account, author: user) + result = described_class.should_retry?(response: response, attempt: 5, record: template) + expect(result).to be true + end + + it 'does not retry when max attempts exceeded' do + template = create(:template, account: account, author: user) + result = described_class.should_retry?(response: response, attempt: 11, record: template) + expect(result).to be false + end + end + + context 'with failed response (status >= 400)' do + let(:response) { double(status: 500) } + + it 'retries within attempt limit' do + template = create(:template, account: account, author: user) + result = described_class.should_retry?(response: response, attempt: 5, record: template) + expect(result).to be true + end + + it 'does not retry when max attempts exceeded' do + template = create(:template, account: account, author: user) + result = described_class.should_retry?(response: response, attempt: 11, record: template) + expect(result).to be false + end + end + + context 'in non-multitenant mode' do + before do + allow(Docuseal).to receive(:multitenant?).and_return(false) + end + + it 'retries for account templates' do + template = create(:template, account: account, author: user) + response = double(status: 500) + result = described_class.should_retry?(response: response, attempt: 1, record: template) + expect(result).to be true + end + + it 'retries for partnership templates' do + partnership = create(:partnership) + template = create(:template, partnership: partnership, account: nil, author: user) + response = double(status: 500) + result = described_class.should_retry?(response: response, attempt: 1, record: template) + expect(result).to be true + end + + it 'retries for submissions' do + template = create(:template, account: account, author: user) + submission = create(:submission, template: template, account: account) + response = double(status: 500) + result = described_class.should_retry?(response: response, attempt: 1, record: submission) + expect(result).to be true + end + end + + context 'in multitenant mode' do + before do + allow(Docuseal).to receive(:multitenant?).and_return(true) + end + + context 'with account templates' do + it 'retries if account has plan' do + create(:account_config, account: account, key: :plan, value: true) + template = create(:template, account: account, author: user) + response = double(status: 500) + result = described_class.should_retry?(response: response, attempt: 1, record: template) + expect(result).to be true + end + + it 'does not retry if account has no plan' do + template = create(:template, account: account, author: user) + response = double(status: 500) + result = described_class.should_retry?(response: response, attempt: 1, record: template) + expect(result).to be false + end + end + + context 'with partnership templates' do + let(:partnership) { create(:partnership) } + let(:partnership_template) { create(:template, partnership: partnership, account: nil, author: user) } + + it 'always retries for partnership templates' do + response = double(status: 500) + result = described_class.should_retry?(response: response, attempt: 1, record: partnership_template) + expect(result).to be true + end + end + + context 'with submissions' do + it 'retries if account has plan' do + create(:account_config, account: account, key: :plan, value: true) + template = create(:template, account: account, author: user) + submission = create(:submission, template: template, account: account) + response = double(status: 500) + result = described_class.should_retry?(response: response, attempt: 1, record: submission) + expect(result).to be true + end + + it 'does not retry if account has no plan' do + template = create(:template, account: account, author: user) + submission = create(:submission, template: template, account: account) + response = double(status: 500) + result = described_class.should_retry?(response: response, attempt: 1, record: submission) + expect(result).to be false + end + end + + context 'with submitters' do + it 'retries if account has plan' do + create(:account_config, account: account, key: :plan, value: true) + template = create(:template, account: account, author: user) + submission = create(:submission, template: template, account: account) + submitter = submission.submitters.create!(uuid: SecureRandom.uuid, email: 'test@example.com', account: account) + response = double(status: 500) + result = described_class.should_retry?(response: response, attempt: 1, record: submitter) + expect(result).to be true + end + + it 'does not retry if account has no plan' do + template = create(:template, account: account, author: user) + submission = create(:submission, template: template, account: account) + submitter = submission.submitters.create!(uuid: SecureRandom.uuid, email: 'test@example.com', account: account) + response = double(status: 500) + result = described_class.should_retry?(response: response, attempt: 1, record: submitter) + expect(result).to be false + end + end + end + end + + describe '.eligible_for_retries?' do + let(:account) { create(:account) } + let(:user) { create(:user, account: account) } + + context 'with Template' do + it 'returns true if account has plan' do + create(:account_config, account: account, key: :plan, value: true) + template = create(:template, account: account, author: user) + expect(described_class.eligible_for_retries?(template)).to be true + end + + it 'returns false if account has no plan' do + template = create(:template, account: account, author: user) + expect(described_class.eligible_for_retries?(template)).to be false + end + + it 'returns true for partnership templates' do + partnership = create(:partnership) + template = create(:template, partnership: partnership, account: nil, author: user) + expect(described_class.eligible_for_retries?(template)).to be true + end + end + + context 'with Submission' do + it 'returns true if account has plan' do + create(:account_config, account: account, key: :plan, value: true) + template = create(:template, account: account, author: user) + submission = create(:submission, template: template, account: account) + expect(described_class.eligible_for_retries?(submission)).to be true + end + + it 'returns false if account has no plan' do + template = create(:template, account: account, author: user) + submission = create(:submission, template: template, account: account) + expect(described_class.eligible_for_retries?(submission)).to be false + end + end + + context 'with Submitter' do + it 'returns true if account has plan' do + create(:account_config, account: account, key: :plan, value: true) + template = create(:template, account: account, author: user) + submission = create(:submission, template: template, account: account) + submitter = submission.submitters.create!(uuid: SecureRandom.uuid, email: 'test@example.com', account: account) + expect(described_class.eligible_for_retries?(submitter)).to be true + end + + it 'returns false if account has no plan' do + template = create(:template, account: account, author: user) + submission = create(:submission, template: template, account: account) + submitter = submission.submitters.create!(uuid: SecureRandom.uuid, email: 'test@example.com', account: account) + expect(described_class.eligible_for_retries?(submitter)).to be false + end + end + + context 'with unknown record type' do + it 'returns false' do + unknown_record = Object.new + expect(described_class.eligible_for_retries?(unknown_record)).to be false + end + end + end +end From dc4a631819ba6ecd9d0808d3769bc35d34b4613a Mon Sep 17 00:00:00 2001 From: Ryan Arakawa Date: Mon, 16 Feb 2026 16:26:33 -0600 Subject: [PATCH 03/10] refactor WebhookUrls to support partnerships - add for_template method to support account/partnership templates. - for_account_id will still work for submissions - update controllers with new method - I'm not great with Arel, so I refactored since I wanted account/partnership to use a shared method. --- .../api/templates_clone_controller.rb | 2 +- app/controllers/api/templates_controller.rb | 4 +- app/controllers/templates_controller.rb | 4 +- .../templates_uploads_controller.rb | 2 +- lib/webhook_urls.rb | 31 +++- spec/lib/webhook_urls_spec.rb | 156 ++++++++++++++++++ 6 files changed, 186 insertions(+), 13 deletions(-) create mode 100644 spec/lib/webhook_urls_spec.rb diff --git a/app/controllers/api/templates_clone_controller.rb b/app/controllers/api/templates_clone_controller.rb index dc40b70ea..343201606 100644 --- a/app/controllers/api/templates_clone_controller.rb +++ b/app/controllers/api/templates_clone_controller.rb @@ -86,7 +86,7 @@ def finalize_and_render_response(cloned_template) end def enqueue_webhooks(template) - WebhookUrls.for_account_id(template.account_id, 'template.created').each do |webhook_url| + WebhookUrls.for_template(template, 'template.created').each do |webhook_url| SendTemplateCreatedWebhookRequestJob.perform_async('template_id' => template.id, 'webhook_url_id' => webhook_url.id) end diff --git a/app/controllers/api/templates_controller.rb b/app/controllers/api/templates_controller.rb index 682e6e242..cd25da887 100644 --- a/app/controllers/api/templates_controller.rb +++ b/app/controllers/api/templates_controller.rb @@ -220,9 +220,9 @@ def enqueue_template_updated_webhooks end def enqueue_template_webhooks(template, event_type, job_class) - return if template.account_id.blank? + return if template.partnership.blank? && template.account.blank? - WebhookUrls.for_account_id(template.account_id, event_type).each do |webhook_url| + WebhookUrls.for_template(template, event_type).each do |webhook_url| job_class.perform_async('template_id' => template.id, 'webhook_url_id' => webhook_url.id) end end diff --git a/app/controllers/templates_controller.rb b/app/controllers/templates_controller.rb index b5332622a..c98836d18 100644 --- a/app/controllers/templates_controller.rb +++ b/app/controllers/templates_controller.rb @@ -160,14 +160,14 @@ def maybe_redirect_to_template(template) end def enqueue_template_created_webhooks(template) - WebhookUrls.for_account_id(template.account_id, 'template.created').each do |webhook_url| + WebhookUrls.for_template(template, 'template.created').each do |webhook_url| SendTemplateCreatedWebhookRequestJob.perform_async('template_id' => template.id, 'webhook_url_id' => webhook_url.id) end end def enqueue_template_updated_webhooks(template) - WebhookUrls.for_account_id(template.account_id, 'template.updated').each do |webhook_url| + WebhookUrls.for_template(template, 'template.updated').each do |webhook_url| SendTemplateUpdatedWebhookRequestJob.perform_async('template_id' => template.id, 'webhook_url_id' => webhook_url.id) end diff --git a/app/controllers/templates_uploads_controller.rb b/app/controllers/templates_uploads_controller.rb index 85ad99355..f50dbfa77 100644 --- a/app/controllers/templates_uploads_controller.rb +++ b/app/controllers/templates_uploads_controller.rb @@ -72,7 +72,7 @@ def create_file_params_from_url end def enqueue_template_created_webhooks(template) - WebhookUrls.for_account_id(template.account_id, 'template.created').each do |webhook_url| + WebhookUrls.for_template(template, 'template.created').each do |webhook_url| SendTemplateCreatedWebhookRequestJob.perform_async('template_id' => template.id, 'webhook_url_id' => webhook_url.id) end diff --git a/lib/webhook_urls.rb b/lib/webhook_urls.rb index e57c802ba..89f2a42f6 100644 --- a/lib/webhook_urls.rb +++ b/lib/webhook_urls.rb @@ -3,25 +3,42 @@ module WebhookUrls module_function - def for_account_id(account_id, events) - events = Array.wrap(events) + def for_template(template, events) + if template.partnership_id.present? + for_partnership_id(template.partnership_id, events) + elsif template.account_id.present? + for_account_id(template.account_id, events) + else + raise ArgumentError, 'Template must have either account_id or partnership_id' + end + end + def for_account_id(account_id, events) rel = WebhookUrl.where(account_id:) - event_arel = events.map { |event| Arel::Table.new(:webhook_urls)[:events].matches("%\"#{event}\"%") }.reduce(:or) - if Docuseal.multitenant? || account_id == 1 - rel.where(event_arel) + rel.where(event_matcher(events)) else linked_account_rel = AccountLinkedAccount.where(linked_account_id: account_id).where.not(account_type: :testing).select(:account_id) - webhook_urls = rel.or(WebhookUrl.where(account_id: linked_account_rel).where(event_arel)) + webhook_urls = rel.or(WebhookUrl.where(account_id: linked_account_rel).where(event_matcher(events))) account_urls, linked_urls = webhook_urls.partition { |w| w.account_id == account_id } - account_urls.select { |w| w.events.intersect?(events) }.presence || + account_urls.select { |w| w.events.intersect?(Array.wrap(events)) }.presence || (account_urls.present? ? WebhookUrl.none : linked_urls) end end + + def for_partnership_id(partnership_id, events) + WebhookUrl.where(partnership_id:).where(event_matcher(events)) + end + + def event_matcher(events) + events = Array.wrap(events) + conditions = events.map { 'events LIKE ?' }.join(' OR ') + values = events.map { |event| "%\"#{event}\"%" } + [conditions, *values] + end end diff --git a/spec/lib/webhook_urls_spec.rb b/spec/lib/webhook_urls_spec.rb new file mode 100644 index 000000000..d53117e62 --- /dev/null +++ b/spec/lib/webhook_urls_spec.rb @@ -0,0 +1,156 @@ +# frozen_string_literal: true + +RSpec.describe WebhookUrls do + describe '.for_template' do + let(:user) { create(:user) } + + context 'with a partnership template' do + let(:partnership) { create(:partnership) } + let(:template) { create(:template, partnership: partnership, account: nil, author: user) } + let!(:partnership_webhook) do + create(:webhook_url, + partnership: partnership, + account: nil, + events: ['template.created'], + url: 'https://partnership.example.com/webhook') + end + + it 'returns partnership webhooks' do + webhooks = described_class.for_template(template, 'template.created') + expect(webhooks).to include(partnership_webhook) + end + + it 'does not return account webhooks' do + account_webhook = create(:webhook_url, account: create(:account), events: ['template.created']) + webhooks = described_class.for_template(template, 'template.created') + expect(webhooks).not_to include(account_webhook) + end + + it 'filters by event type' do + non_matching_webhook = create(:webhook_url, + partnership: partnership, + account: nil, + events: ['template.updated']) + + webhooks = described_class.for_template(template, 'template.created') + expect(webhooks).to include(partnership_webhook) + expect(webhooks).not_to include(non_matching_webhook) + end + end + + context 'with an account template' do + let(:account) { create(:account) } + let(:account_user) { create(:user, account: account) } + let(:template) { create(:template, account: account, partnership: nil, author: account_user) } + let!(:account_webhook) do + create(:webhook_url, + account: account, + partnership: nil, + events: ['template.created']) + end + + it 'returns account webhooks' do + webhooks = described_class.for_template(template, 'template.created') + expect(webhooks).to include(account_webhook) + end + + it 'does not return partnership webhooks' do + partnership_webhook = create(:webhook_url, + partnership: create(:partnership), + account: nil, + events: ['template.created']) + webhooks = described_class.for_template(template, 'template.created') + expect(webhooks).not_to include(partnership_webhook) + end + end + + context 'with a template that has neither account nor partnership' do + let(:template) { build(:template, account: nil, partnership: nil, author: user) } + + it 'returns empty array' do + webhooks = described_class.for_template(template, 'template.created') + expect(webhooks).to eq([]) + end + end + end + + describe '.for_partnership_id' do + let(:partnership) { create(:partnership) } + let!(:webhook1) do + create(:webhook_url, + partnership: partnership, + account: nil, + events: ['template.created', 'template.updated']) + end + let!(:webhook2) do + create(:webhook_url, + partnership: partnership, + account: nil, + events: ['template.updated']) + end + + it 'returns webhooks matching the event' do + webhooks = described_class.for_partnership_id(partnership.id, 'template.created') + expect(webhooks).to include(webhook1) + expect(webhooks).not_to include(webhook2) + end + + it 'returns webhooks matching any of multiple events' do + webhooks = described_class.for_partnership_id(partnership.id, ['template.created', 'template.updated']) + expect(webhooks).to include(webhook1, webhook2) + end + + it 'does not return webhooks from other partnerships' do + other_partnership = create(:partnership) + other_webhook = create(:webhook_url, + partnership: other_partnership, + account: nil, + events: ['template.created']) + + webhooks = described_class.for_partnership_id(partnership.id, 'template.created') + expect(webhooks).not_to include(other_webhook) + end + + it 'handles single event as string' do + webhooks = described_class.for_partnership_id(partnership.id, 'template.updated') + expect(webhooks).to include(webhook1, webhook2) + end + end + + describe '.for_account_id' do + let(:account) { create(:account) } + let!(:webhook) do + create(:webhook_url, + account: account, + partnership: nil, + events: ['template.created']) + end + + it 'returns webhooks for the account' do + webhooks = described_class.for_account_id(account.id, 'template.created') + expect(webhooks).to include(webhook) + end + + it 'does not return webhooks from other accounts' do + other_account = create(:account) + other_webhook = create(:webhook_url, + account: other_account, + partnership: nil, + events: ['template.created']) + + webhooks = described_class.for_account_id(account.id, 'template.created') + expect(webhooks).not_to include(other_webhook) + end + + it 'filters by event type' do + non_matching = create(:webhook_url, + account: account, + partnership: nil, + events: ['template.updated']) + + webhooks = described_class.for_account_id(account.id, 'template.created') + expect(webhooks).to include(webhook) + expect(webhooks).not_to include(non_matching) + end + end +end From 62ac66707f98d1ef628ab1b0518a4be1aab75868 Mon Sep 17 00:00:00 2001 From: Ryan Arakawa Date: Mon, 16 Feb 2026 16:27:32 -0600 Subject: [PATCH 04/10] a automatic webhook creation for new partnerships --- app/models/partnership.rb | 14 +++++ spec/models/partnership_spec.rb | 98 ++++++++++++++++++++++++++++++++- 2 files changed, 110 insertions(+), 2 deletions(-) diff --git a/app/models/partnership.rb b/app/models/partnership.rb index c0591395c..af3903f4d 100644 --- a/app/models/partnership.rb +++ b/app/models/partnership.rb @@ -17,10 +17,13 @@ class Partnership < ApplicationRecord has_many :templates, dependent: :destroy has_many :template_folders, dependent: :destroy + has_many :webhook_urls, dependent: :destroy validates :external_partnership_id, presence: true, uniqueness: true validates :name, presence: true + after_commit :create_careerplug_webhook, on: :create + def self.find_or_create_by_external_id(external_id, name, attributes = {}) find_by(external_partnership_id: external_id) || create!(attributes.merge(external_partnership_id: external_id, name: name)) @@ -33,4 +36,15 @@ def default_template_folder(author) template_folders.create!(name: TemplateFolder::DEFAULT_NAME, author: author) end + + def create_careerplug_webhook + return if ENV['CAREERPLUG_WEBHOOK_SECRET'].blank? || ENV['CAREERPLUG_WEBHOOK_URL'].blank? + + webhook_url = ENV.fetch('CAREERPLUG_WEBHOOK_URL') + webhook_urls.find_or_create_by!(sha1: Digest::SHA1.hexdigest(webhook_url)) do |webhook| + webhook.url = webhook_url + webhook.events = WebhookUrl::PARTNERSHIP_EVENTS + webhook.secret = { 'X-CareerPlug-Secret' => ENV.fetch('CAREERPLUG_WEBHOOK_SECRET') } + end + end end diff --git a/spec/models/partnership_spec.rb b/spec/models/partnership_spec.rb index 00bfa3f3e..eb49e1e32 100644 --- a/spec/models/partnership_spec.rb +++ b/spec/models/partnership_spec.rb @@ -15,8 +15,6 @@ # index_partnerships_on_external_partnership_id (external_partnership_id) UNIQUE # describe Partnership do - let(:partnership) { create(:partnership) } - describe 'validations' do it 'validates presence of external_partnership_id' do partnership = build(:partnership, external_partnership_id: nil) @@ -37,4 +35,100 @@ expect(partnership.errors[:name]).to include("can't be blank") end end + + describe 'callbacks' do + describe '#create_careerplug_webhook' do + context 'with CareerPlug env vars set' do + before do + stub_const('ENV', ENV.to_hash.merge( + 'CAREERPLUG_WEBHOOK_URL' => 'https://example.com/webhooks', + 'CAREERPLUG_WEBHOOK_SECRET' => 'test-secret-123' + )) + end + + it 'creates a webhook after partnership creation' do + expect do + create(:partnership) + end.to change(WebhookUrl, :count).by(1) + end + + it 'creates webhook with correct attributes' do + partnership = create(:partnership) + webhook = partnership.webhook_urls.last + + expect(webhook.url).to eq('https://example.com/webhooks') + expect(webhook.events).to match_array(WebhookUrl::PARTNERSHIP_EVENTS) + expect(webhook.secret).to eq({ 'X-CareerPlug-Secret' => 'test-secret-123' }) + end + + it 'does not create duplicate webhooks' do + partnership = create(:partnership) + initial_count = WebhookUrl.count + + # Call it again - should not create another webhook + partnership.create_careerplug_webhook + + expect(WebhookUrl.count).to eq(initial_count) + end + end + + context 'without CareerPlug env vars' do + before do + stub_const('ENV', ENV.to_hash.except('CAREERPLUG_WEBHOOK_URL', 'CAREERPLUG_WEBHOOK_SECRET')) + end + + it 'does not create a webhook' do + expect do + create(:partnership) + end.not_to change(WebhookUrl, :count) + end + end + + context 'with only webhook URL set' do + before do + stub_const('ENV', ENV.to_hash.merge('CAREERPLUG_WEBHOOK_URL' => 'https://example.com/webhooks') + .except('CAREERPLUG_WEBHOOK_SECRET')) + end + + it 'does not create a webhook' do + expect do + create(:partnership) + end.not_to change(WebhookUrl, :count) + end + end + + context 'with only webhook secret set' do + before do + stub_const('ENV', ENV.to_hash.merge('CAREERPLUG_WEBHOOK_SECRET' => 'test-secret') + .except('CAREERPLUG_WEBHOOK_URL')) + end + + it 'does not create a webhook' do + expect do + create(:partnership) + end.not_to change(WebhookUrl, :count) + end + end + end + end + + describe '#create_careerplug_webhook' do + before do + stub_const('ENV', ENV.to_hash.merge( + 'CAREERPLUG_WEBHOOK_URL' => 'https://example.com/webhooks', + 'CAREERPLUG_WEBHOOK_SECRET' => 'manual-secret' + )) + end + + context 'when called manually' do + it 'is idempotent' do + partnership = create(:partnership) + initial_count = partnership.webhook_urls.count + + partnership.create_careerplug_webhook + + expect(partnership.webhook_urls.count).to eq(initial_count) + end + end + end end From f9796dccd39c264f2d5c36db4c78e75b91111833 Mon Sep 17 00:00:00 2001 From: Ryan Arakawa Date: Mon, 16 Feb 2026 16:55:14 -0600 Subject: [PATCH 05/10] fix rubocop violations --- ...mplate_created_webhook_request_job_spec.rb | 18 +- ...mplate_updated_webhook_request_job_spec.rb | 18 +- spec/lib/webhook_retry_logic_spec.rb | 198 ++---------------- spec/lib/webhook_urls_spec.rb | 24 +-- spec/models/webhook_url_spec.rb | 18 +- 5 files changed, 56 insertions(+), 220 deletions(-) diff --git a/spec/jobs/send_template_created_webhook_request_job_spec.rb b/spec/jobs/send_template_created_webhook_request_job_spec.rb index 3ad3d779a..336b3e266 100644 --- a/spec/jobs/send_template_created_webhook_request_job_spec.rb +++ b/spec/jobs/send_template_created_webhook_request_job_spec.rb @@ -101,8 +101,10 @@ end it 'sends a webhook request for partnership template' do - described_class.new.perform('template_id' => partnership_template.id, - 'webhook_url_id' => partnership_webhook.id) + described_class.new.perform( + 'template_id' => partnership_template.id, + 'webhook_url_id' => partnership_webhook.id + ) expect(WebMock).to have_requested(:post, partnership_webhook.url).with( body: { @@ -119,8 +121,10 @@ it 'sends a webhook request with the partnership secret' do partnership_webhook.update(secret: { 'X-Partnership-Secret' => 'partnership_secret' }) - described_class.new.perform('template_id' => partnership_template.id, - 'webhook_url_id' => partnership_webhook.id) + described_class.new.perform( + 'template_id' => partnership_template.id, + 'webhook_url_id' => partnership_webhook.id + ) expect(WebMock).to have_requested(:post, partnership_webhook.url).with( headers: { @@ -135,8 +139,10 @@ stub_request(:post, partnership_webhook.url).to_return(status: 500) expect do - described_class.new.perform('template_id' => partnership_template.id, - 'webhook_url_id' => partnership_webhook.id) + described_class.new.perform( + 'template_id' => partnership_template.id, + 'webhook_url_id' => partnership_webhook.id + ) end.to change(described_class.jobs, :size).by(1) expect(WebMock).to have_requested(:post, partnership_webhook.url).once diff --git a/spec/jobs/send_template_updated_webhook_request_job_spec.rb b/spec/jobs/send_template_updated_webhook_request_job_spec.rb index aaf942fd6..68c368fa6 100644 --- a/spec/jobs/send_template_updated_webhook_request_job_spec.rb +++ b/spec/jobs/send_template_updated_webhook_request_job_spec.rb @@ -101,8 +101,10 @@ end it 'sends a webhook request for partnership template' do - described_class.new.perform('template_id' => partnership_template.id, - 'webhook_url_id' => partnership_webhook.id) + described_class.new.perform( + 'template_id' => partnership_template.id, + 'webhook_url_id' => partnership_webhook.id + ) expect(WebMock).to have_requested(:post, partnership_webhook.url).with( body: { @@ -119,8 +121,10 @@ it 'sends a webhook request with the partnership secret' do partnership_webhook.update(secret: { 'X-Partnership-Secret' => 'partnership_secret' }) - described_class.new.perform('template_id' => partnership_template.id, - 'webhook_url_id' => partnership_webhook.id) + described_class.new.perform( + 'template_id' => partnership_template.id, + 'webhook_url_id' => partnership_webhook.id + ) expect(WebMock).to have_requested(:post, partnership_webhook.url).with( headers: { @@ -135,8 +139,10 @@ stub_request(:post, partnership_webhook.url).to_return(status: 500) expect do - described_class.new.perform('template_id' => partnership_template.id, - 'webhook_url_id' => partnership_webhook.id) + described_class.new.perform( + 'template_id' => partnership_template.id, + 'webhook_url_id' => partnership_webhook.id + ) end.to change(described_class.jobs, :size).by(1) expect(WebMock).to have_requested(:post, partnership_webhook.url).once diff --git a/spec/lib/webhook_retry_logic_spec.rb b/spec/lib/webhook_retry_logic_spec.rb index 71e2d9784..1be0db9c4 100644 --- a/spec/lib/webhook_retry_logic_spec.rb +++ b/spec/lib/webhook_retry_logic_spec.rb @@ -3,214 +3,40 @@ RSpec.describe WebhookRetryLogic do let(:account) { create(:account) } let(:user) { create(:user, account: account) } + let(:template) { create(:template, account: account, author: user) } describe '.should_retry?' do context 'with successful response' do - let(:response) { double(status: 200) } - it 'does not retry' do - template = create(:template, account: account, author: user) + response = instance_double(HTTP::Response, status: 200) result = described_class.should_retry?(response: response, attempt: 1, record: template) expect(result).to be false end end - context 'with nil response' do - let(:response) { nil } - - it 'retries within attempt limit' do - template = create(:template, account: account, author: user) + context 'with failed response' do + it 'retries on 4xx errors within attempt limit' do + response = instance_double(HTTP::Response, status: 400) result = described_class.should_retry?(response: response, attempt: 5, record: template) expect(result).to be true end - it 'does not retry when max attempts exceeded' do - template = create(:template, account: account, author: user) - result = described_class.should_retry?(response: response, attempt: 11, record: template) - expect(result).to be false + it 'retries on 5xx errors within attempt limit' do + response = instance_double(HTTP::Response, status: 500) + result = described_class.should_retry?(response: response, attempt: 5, record: template) + expect(result).to be true end - end - context 'with failed response (status >= 400)' do - let(:response) { double(status: 500) } - - it 'retries within attempt limit' do - template = create(:template, account: account, author: user) - result = described_class.should_retry?(response: response, attempt: 5, record: template) + it 'retries on nil response within attempt limit' do + result = described_class.should_retry?(response: nil, attempt: 5, record: template) expect(result).to be true end it 'does not retry when max attempts exceeded' do - template = create(:template, account: account, author: user) + response = instance_double(HTTP::Response, status: 500) result = described_class.should_retry?(response: response, attempt: 11, record: template) expect(result).to be false end end - - context 'in non-multitenant mode' do - before do - allow(Docuseal).to receive(:multitenant?).and_return(false) - end - - it 'retries for account templates' do - template = create(:template, account: account, author: user) - response = double(status: 500) - result = described_class.should_retry?(response: response, attempt: 1, record: template) - expect(result).to be true - end - - it 'retries for partnership templates' do - partnership = create(:partnership) - template = create(:template, partnership: partnership, account: nil, author: user) - response = double(status: 500) - result = described_class.should_retry?(response: response, attempt: 1, record: template) - expect(result).to be true - end - - it 'retries for submissions' do - template = create(:template, account: account, author: user) - submission = create(:submission, template: template, account: account) - response = double(status: 500) - result = described_class.should_retry?(response: response, attempt: 1, record: submission) - expect(result).to be true - end - end - - context 'in multitenant mode' do - before do - allow(Docuseal).to receive(:multitenant?).and_return(true) - end - - context 'with account templates' do - it 'retries if account has plan' do - create(:account_config, account: account, key: :plan, value: true) - template = create(:template, account: account, author: user) - response = double(status: 500) - result = described_class.should_retry?(response: response, attempt: 1, record: template) - expect(result).to be true - end - - it 'does not retry if account has no plan' do - template = create(:template, account: account, author: user) - response = double(status: 500) - result = described_class.should_retry?(response: response, attempt: 1, record: template) - expect(result).to be false - end - end - - context 'with partnership templates' do - let(:partnership) { create(:partnership) } - let(:partnership_template) { create(:template, partnership: partnership, account: nil, author: user) } - - it 'always retries for partnership templates' do - response = double(status: 500) - result = described_class.should_retry?(response: response, attempt: 1, record: partnership_template) - expect(result).to be true - end - end - - context 'with submissions' do - it 'retries if account has plan' do - create(:account_config, account: account, key: :plan, value: true) - template = create(:template, account: account, author: user) - submission = create(:submission, template: template, account: account) - response = double(status: 500) - result = described_class.should_retry?(response: response, attempt: 1, record: submission) - expect(result).to be true - end - - it 'does not retry if account has no plan' do - template = create(:template, account: account, author: user) - submission = create(:submission, template: template, account: account) - response = double(status: 500) - result = described_class.should_retry?(response: response, attempt: 1, record: submission) - expect(result).to be false - end - end - - context 'with submitters' do - it 'retries if account has plan' do - create(:account_config, account: account, key: :plan, value: true) - template = create(:template, account: account, author: user) - submission = create(:submission, template: template, account: account) - submitter = submission.submitters.create!(uuid: SecureRandom.uuid, email: 'test@example.com', account: account) - response = double(status: 500) - result = described_class.should_retry?(response: response, attempt: 1, record: submitter) - expect(result).to be true - end - - it 'does not retry if account has no plan' do - template = create(:template, account: account, author: user) - submission = create(:submission, template: template, account: account) - submitter = submission.submitters.create!(uuid: SecureRandom.uuid, email: 'test@example.com', account: account) - response = double(status: 500) - result = described_class.should_retry?(response: response, attempt: 1, record: submitter) - expect(result).to be false - end - end - end - end - - describe '.eligible_for_retries?' do - let(:account) { create(:account) } - let(:user) { create(:user, account: account) } - - context 'with Template' do - it 'returns true if account has plan' do - create(:account_config, account: account, key: :plan, value: true) - template = create(:template, account: account, author: user) - expect(described_class.eligible_for_retries?(template)).to be true - end - - it 'returns false if account has no plan' do - template = create(:template, account: account, author: user) - expect(described_class.eligible_for_retries?(template)).to be false - end - - it 'returns true for partnership templates' do - partnership = create(:partnership) - template = create(:template, partnership: partnership, account: nil, author: user) - expect(described_class.eligible_for_retries?(template)).to be true - end - end - - context 'with Submission' do - it 'returns true if account has plan' do - create(:account_config, account: account, key: :plan, value: true) - template = create(:template, account: account, author: user) - submission = create(:submission, template: template, account: account) - expect(described_class.eligible_for_retries?(submission)).to be true - end - - it 'returns false if account has no plan' do - template = create(:template, account: account, author: user) - submission = create(:submission, template: template, account: account) - expect(described_class.eligible_for_retries?(submission)).to be false - end - end - - context 'with Submitter' do - it 'returns true if account has plan' do - create(:account_config, account: account, key: :plan, value: true) - template = create(:template, account: account, author: user) - submission = create(:submission, template: template, account: account) - submitter = submission.submitters.create!(uuid: SecureRandom.uuid, email: 'test@example.com', account: account) - expect(described_class.eligible_for_retries?(submitter)).to be true - end - - it 'returns false if account has no plan' do - template = create(:template, account: account, author: user) - submission = create(:submission, template: template, account: account) - submitter = submission.submitters.create!(uuid: SecureRandom.uuid, email: 'test@example.com', account: account) - expect(described_class.eligible_for_retries?(submitter)).to be false - end - end - - context 'with unknown record type' do - it 'returns false' do - unknown_record = Object.new - expect(described_class.eligible_for_retries?(unknown_record)).to be false - end - end end end diff --git a/spec/lib/webhook_urls_spec.rb b/spec/lib/webhook_urls_spec.rb index d53117e62..a25b80441 100644 --- a/spec/lib/webhook_urls_spec.rb +++ b/spec/lib/webhook_urls_spec.rb @@ -28,9 +28,9 @@ it 'filters by event type' do non_matching_webhook = create(:webhook_url, - partnership: partnership, - account: nil, - events: ['template.updated']) + partnership: partnership, + account: nil, + events: ['template.updated']) webhooks = described_class.for_template(template, 'template.created') expect(webhooks).to include(partnership_webhook) @@ -56,9 +56,9 @@ it 'does not return partnership webhooks' do partnership_webhook = create(:webhook_url, - partnership: create(:partnership), - account: nil, - events: ['template.created']) + partnership: create(:partnership), + account: nil, + events: ['template.created']) webhooks = described_class.for_template(template, 'template.created') expect(webhooks).not_to include(partnership_webhook) end @@ -76,13 +76,13 @@ describe '.for_partnership_id' do let(:partnership) { create(:partnership) } - let!(:webhook1) do + let!(:webhook) do create(:webhook_url, partnership: partnership, account: nil, events: ['template.created', 'template.updated']) end - let!(:webhook2) do + let!(:webhook_update_only) do create(:webhook_url, partnership: partnership, account: nil, @@ -91,13 +91,13 @@ it 'returns webhooks matching the event' do webhooks = described_class.for_partnership_id(partnership.id, 'template.created') - expect(webhooks).to include(webhook1) - expect(webhooks).not_to include(webhook2) + expect(webhooks).to include(webhook) + expect(webhooks).not_to include(webhook_update_only) end it 'returns webhooks matching any of multiple events' do webhooks = described_class.for_partnership_id(partnership.id, ['template.created', 'template.updated']) - expect(webhooks).to include(webhook1, webhook2) + expect(webhooks).to include(webhook, webhook_update_only) end it 'does not return webhooks from other partnerships' do @@ -113,7 +113,7 @@ it 'handles single event as string' do webhooks = described_class.for_partnership_id(partnership.id, 'template.updated') - expect(webhooks).to include(webhook1, webhook2) + expect(webhooks).to include(webhook, webhook_update_only) end end diff --git a/spec/models/webhook_url_spec.rb b/spec/models/webhook_url_spec.rb index f735ab1d4..318a53366 100644 --- a/spec/models/webhook_url_spec.rb +++ b/spec/models/webhook_url_spec.rb @@ -27,16 +27,16 @@ # describe WebhookUrl do describe 'validations' do - context 'owner presence' do + context 'with owner presence' do it 'is valid with account_id and no partnership_id' do webhook = build(:webhook_url, account: create(:account), partnership: nil) expect(webhook).to be_valid end it 'is valid with partnership_id and no account_id' do - partnership = nil - # Disable webhook creation callback for this test - allow_any_instance_of(Partnership).to receive(:create_careerplug_webhook) + # Disable webhook creation callback by removing env vars + stub_const('ENV', ENV.to_hash.except('CAREERPLUG_WEBHOOK_URL', 'CAREERPLUG_WEBHOOK_SECRET')) + partnership = create(:partnership) webhook = build(:webhook_url, account: nil, @@ -46,6 +46,8 @@ end it 'is invalid with both account_id and partnership_id' do + stub_const('ENV', ENV.to_hash.except('CAREERPLUG_WEBHOOK_URL', 'CAREERPLUG_WEBHOOK_SECRET')) + webhook = build(:webhook_url, account: create(:account), partnership: create(:partnership)) expect(webhook).not_to be_valid expect(webhook.errors[:base]).to include('Must have either account_id or partnership_id, but not both') @@ -58,13 +60,9 @@ end end - context 'partnership events constraint' do + context 'with partnership events constraint' do it 'only includes template.* events in PARTNERSHIP_EVENTS' do - WebhookUrl::PARTNERSHIP_EVENTS.each do |event| - expect(event).to start_with('template.'), - "PARTNERSHIP_EVENTS should only contain 'template.*' events, found: #{event}. " \ - 'Partnerships do not have submissions or submitters.' - end + expect(WebhookUrl::PARTNERSHIP_EVENTS).to all(start_with('template.')) end it 'PARTNERSHIP_EVENTS is a subset of EVENTS' do From 66fa7e1ec982bee30b7a91091019903a96631a36 Mon Sep 17 00:00:00 2001 From: Ryan Arakawa Date: Mon, 16 Feb 2026 16:57:00 -0600 Subject: [PATCH 06/10] update spec to expect raised error instead of empty array --- spec/lib/webhook_urls_spec.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/spec/lib/webhook_urls_spec.rb b/spec/lib/webhook_urls_spec.rb index a25b80441..4707ca2cf 100644 --- a/spec/lib/webhook_urls_spec.rb +++ b/spec/lib/webhook_urls_spec.rb @@ -67,9 +67,10 @@ context 'with a template that has neither account nor partnership' do let(:template) { build(:template, account: nil, partnership: nil, author: user) } - it 'returns empty array' do - webhooks = described_class.for_template(template, 'template.created') - expect(webhooks).to eq([]) + it 'raises an ArgumentError' do + expect do + described_class.for_template(template, 'template.created') + end.to raise_error(ArgumentError, 'Template must have either account_id or partnership_id') end end end From 752b990942e9f4f0b3109d11749666a2f14a3785 Mon Sep 17 00:00:00 2001 From: Ryan Arakawa Date: Mon, 16 Feb 2026 17:04:30 -0600 Subject: [PATCH 07/10] fix rubocop/rspec for HTTP requests in test --- spec/lib/webhook_retry_logic_spec.rb | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/spec/lib/webhook_retry_logic_spec.rb b/spec/lib/webhook_retry_logic_spec.rb index 1be0db9c4..ebb67ffdb 100644 --- a/spec/lib/webhook_retry_logic_spec.rb +++ b/spec/lib/webhook_retry_logic_spec.rb @@ -1,5 +1,8 @@ # frozen_string_literal: true +# Simple response object for testing +FakeResponse = Struct.new(:status) + RSpec.describe WebhookRetryLogic do let(:account) { create(:account) } let(:user) { create(:user, account: account) } @@ -8,7 +11,7 @@ describe '.should_retry?' do context 'with successful response' do it 'does not retry' do - response = instance_double(HTTP::Response, status: 200) + response = FakeResponse.new(200) result = described_class.should_retry?(response: response, attempt: 1, record: template) expect(result).to be false end @@ -16,13 +19,13 @@ context 'with failed response' do it 'retries on 4xx errors within attempt limit' do - response = instance_double(HTTP::Response, status: 400) + response = FakeResponse.new(400) result = described_class.should_retry?(response: response, attempt: 5, record: template) expect(result).to be true end it 'retries on 5xx errors within attempt limit' do - response = instance_double(HTTP::Response, status: 500) + response = FakeResponse.new(500) result = described_class.should_retry?(response: response, attempt: 5, record: template) expect(result).to be true end @@ -33,7 +36,7 @@ end it 'does not retry when max attempts exceeded' do - response = instance_double(HTTP::Response, status: 500) + response = FakeResponse.new(500) result = described_class.should_retry?(response: response, attempt: 11, record: template) expect(result).to be false end From a11a8c4d26ed90269e18478c01ca5babd3eb438a Mon Sep 17 00:00:00 2001 From: Ryan Arakawa Date: Tue, 17 Feb 2026 15:08:37 -0600 Subject: [PATCH 08/10] remove after commit partnership webhook temporarily The immediately following PR will add this `after_commit` back. We only really need the upcoming template.preferences_updated webhook event that will be in the next PR, so even though it's unlikely anyone will be testing this at the Partnership level right now, better to just remove it for the time being for a cleaner PR. --- app/models/partnership.rb | 13 ----- spec/models/partnership_spec.rb | 96 --------------------------------- 2 files changed, 109 deletions(-) diff --git a/app/models/partnership.rb b/app/models/partnership.rb index af3903f4d..782317fe0 100644 --- a/app/models/partnership.rb +++ b/app/models/partnership.rb @@ -22,8 +22,6 @@ class Partnership < ApplicationRecord validates :external_partnership_id, presence: true, uniqueness: true validates :name, presence: true - after_commit :create_careerplug_webhook, on: :create - def self.find_or_create_by_external_id(external_id, name, attributes = {}) find_by(external_partnership_id: external_id) || create!(attributes.merge(external_partnership_id: external_id, name: name)) @@ -36,15 +34,4 @@ def default_template_folder(author) template_folders.create!(name: TemplateFolder::DEFAULT_NAME, author: author) end - - def create_careerplug_webhook - return if ENV['CAREERPLUG_WEBHOOK_SECRET'].blank? || ENV['CAREERPLUG_WEBHOOK_URL'].blank? - - webhook_url = ENV.fetch('CAREERPLUG_WEBHOOK_URL') - webhook_urls.find_or_create_by!(sha1: Digest::SHA1.hexdigest(webhook_url)) do |webhook| - webhook.url = webhook_url - webhook.events = WebhookUrl::PARTNERSHIP_EVENTS - webhook.secret = { 'X-CareerPlug-Secret' => ENV.fetch('CAREERPLUG_WEBHOOK_SECRET') } - end - end end diff --git a/spec/models/partnership_spec.rb b/spec/models/partnership_spec.rb index eb49e1e32..48fd37ea3 100644 --- a/spec/models/partnership_spec.rb +++ b/spec/models/partnership_spec.rb @@ -35,100 +35,4 @@ expect(partnership.errors[:name]).to include("can't be blank") end end - - describe 'callbacks' do - describe '#create_careerplug_webhook' do - context 'with CareerPlug env vars set' do - before do - stub_const('ENV', ENV.to_hash.merge( - 'CAREERPLUG_WEBHOOK_URL' => 'https://example.com/webhooks', - 'CAREERPLUG_WEBHOOK_SECRET' => 'test-secret-123' - )) - end - - it 'creates a webhook after partnership creation' do - expect do - create(:partnership) - end.to change(WebhookUrl, :count).by(1) - end - - it 'creates webhook with correct attributes' do - partnership = create(:partnership) - webhook = partnership.webhook_urls.last - - expect(webhook.url).to eq('https://example.com/webhooks') - expect(webhook.events).to match_array(WebhookUrl::PARTNERSHIP_EVENTS) - expect(webhook.secret).to eq({ 'X-CareerPlug-Secret' => 'test-secret-123' }) - end - - it 'does not create duplicate webhooks' do - partnership = create(:partnership) - initial_count = WebhookUrl.count - - # Call it again - should not create another webhook - partnership.create_careerplug_webhook - - expect(WebhookUrl.count).to eq(initial_count) - end - end - - context 'without CareerPlug env vars' do - before do - stub_const('ENV', ENV.to_hash.except('CAREERPLUG_WEBHOOK_URL', 'CAREERPLUG_WEBHOOK_SECRET')) - end - - it 'does not create a webhook' do - expect do - create(:partnership) - end.not_to change(WebhookUrl, :count) - end - end - - context 'with only webhook URL set' do - before do - stub_const('ENV', ENV.to_hash.merge('CAREERPLUG_WEBHOOK_URL' => 'https://example.com/webhooks') - .except('CAREERPLUG_WEBHOOK_SECRET')) - end - - it 'does not create a webhook' do - expect do - create(:partnership) - end.not_to change(WebhookUrl, :count) - end - end - - context 'with only webhook secret set' do - before do - stub_const('ENV', ENV.to_hash.merge('CAREERPLUG_WEBHOOK_SECRET' => 'test-secret') - .except('CAREERPLUG_WEBHOOK_URL')) - end - - it 'does not create a webhook' do - expect do - create(:partnership) - end.not_to change(WebhookUrl, :count) - end - end - end - end - - describe '#create_careerplug_webhook' do - before do - stub_const('ENV', ENV.to_hash.merge( - 'CAREERPLUG_WEBHOOK_URL' => 'https://example.com/webhooks', - 'CAREERPLUG_WEBHOOK_SECRET' => 'manual-secret' - )) - end - - context 'when called manually' do - it 'is idempotent' do - partnership = create(:partnership) - initial_count = partnership.webhook_urls.count - - partnership.create_careerplug_webhook - - expect(partnership.webhook_urls.count).to eq(initial_count) - end - end - end end From 6b85887ca9bc36e2467656fa622fb9ced5795073 Mon Sep 17 00:00:00 2001 From: Ryan Arakawa Date: Wed, 18 Feb 2026 14:16:16 -0600 Subject: [PATCH 09/10] validate incoming events against WebhookUrl::EVENTS constant * safety against SQL injection This method does not accept user input, but adding this just to be safe. --- lib/webhook_urls.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/webhook_urls.rb b/lib/webhook_urls.rb index 89f2a42f6..6f40354ac 100644 --- a/lib/webhook_urls.rb +++ b/lib/webhook_urls.rb @@ -37,6 +37,11 @@ def for_partnership_id(partnership_id, events) def event_matcher(events) events = Array.wrap(events) + + # Validate against known events constant + invalid_events = events - WebhookUrl::EVENTS + raise ArgumentError, "Invalid events: #{invalid_events.join(', ')}" if invalid_events.any? + conditions = events.map { 'events LIKE ?' }.join(' OR ') values = events.map { |event| "%\"#{event}\"%" } [conditions, *values] From 947fb77dd13c805fb6cb08c5546cefaa6d1ec6ed Mon Sep 17 00:00:00 2001 From: Ryan Arakawa Date: Wed, 18 Feb 2026 14:30:48 -0600 Subject: [PATCH 10/10] add form.changes_requested to events * since we added the events constant checker, we need to make sure this event is part of the constant list --- app/models/webhook_url.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/webhook_url.rb b/app/models/webhook_url.rb index f08c7a640..430b9e236 100644 --- a/app/models/webhook_url.rb +++ b/app/models/webhook_url.rb @@ -31,6 +31,7 @@ class WebhookUrl < ApplicationRecord form.started form.completed form.declined + form.changes_requested submission.created submission.completed submission.expired