Create new table for instance hooks and new NoStiSystemHook model
What does this MR do and why?
As part of the Cells work, we need to extract system hooks to their own table.
As a SystemHook model already exists, which uses STI, I have instead created a temporary NoStiSystemHook model, which uses the new system_hooks table. The intention is that we can later duplicate records into the new table, and use the new model as needed, until we are able to switch over completely. At which point we will make the current SystemHook model not use STI and instead use the concern that the new model uses.
The table should be the same as the existing web_hooks table, except there is no project_id, group_id, integration_id or type since these are not needed in this table. In instance_integrations the tables were kept identical, but I don't believe it's needed here, as the issues that led to the tables being made identical there do not apply for the web hooks.
Since a lot of the work done in this MR is moving code (e.g. app/models/hooks/web_hook.rb) or copying code (e.g. app/models/hooks/no_sti_system_hook.rb), reviewing this code can be a bit tricky. I suggest for the copied/moved files you check out the files in master and use the regular diff program to compare them:
git checkout master
git checkout 474818-create-new-table-model-for-instance-hooks -- \
app/models/concerns/web_hooks/hook.rb \
spec/models/hooks/no_sti_system_hook_spec.rb \
spec/support/shared_examples/models/concerns/web_hooks/web_hook_shared_examples.rb
diff -w --color --unified app/models/hooks/web_hook.rb app/models/concerns/web_hooks/hook.rb
diff -w --color --unified spec/models/hooks/system_hook_spec.rb spec/models/hooks/no_sti_system_hook_spec.rb
diff -w --color --unified spec/models/hooks/web_hook_spec.rb spec/support/shared_examples/models/concerns/web_hooks/web_hook_shared_examples.rbDiff output (last edited 2025-01-13)
$ diff -w --color --unified app/models/hooks/web_hook.rb app/models/concerns/web_hooks/hook.rb--- app/models/hooks/web_hook.rb 2025-01-13 17:55:14.062594447 +0000
+++ app/models/concerns/web_hooks/hook.rb 2025-01-13 17:55:14.169593057 +0000
@@ -1,13 +1,20 @@
# frozen_string_literal: true
-class WebHook < ApplicationRecord
- include Sortable
- include WebHooks::AutoDisabling
+module WebHooks
+ module Hook
+ extend ActiveSupport::Concern
InterpolationError = Class.new(StandardError)
SECRET_MASK = '************'
+ # See app/validators/json_schemas/web_hooks_url_variables.json
+ VARIABLE_REFERENCE_RE = /\{([A-Za-z]+[0-9]*(?:[._-][A-Za-z0-9]+)*)\}/
+
+ included do
+ include Sortable
+ include WebHooks::AutoDisabling
+
attr_encrypted :token,
mode: :per_attribute_iv,
algorithm: 'aes-256-gcm',
@@ -36,12 +43,11 @@
encode: false,
encode_iv: false
- has_many :web_hook_logs
-
validates :url, presence: true
- validates :url, public_url: true, unless: ->(hook) { hook.is_a?(SystemHook) || hook.url_variables? }
+ validates :url, public_url: true, if: ->(hook) { hook.validate_public_url? && !hook.url_variables? }
validates :token, format: { without: /\n/ }
+
after_initialize :initialize_url_variables
after_initialize :initialize_custom_headers
@@ -50,7 +56,9 @@
before_validation :reset_custom_headers, unless: ->(hook) { hook.is_a?(ServiceHook) }, on: :update
before_validation :set_branch_filter_nil, if: :branch_filter_strategy_all_branches?
validates :push_events_branch_filter, untrusted_regexp: true, if: :branch_filter_strategy_regex?
- validates :push_events_branch_filter, "web_hooks/wildcard_branch_filter": true, if: :branch_filter_strategy_wildcard?
+ validates(
+ :push_events_branch_filter, "web_hooks/wildcard_branch_filter": true, if: :branch_filter_strategy_wildcard?
+ )
validates :url_variables, json_schema: { filename: 'web_hooks_url_variables' }
validate :no_missing_url_variables
@@ -58,24 +66,20 @@
validates :custom_headers, json_schema: { filename: 'web_hooks_custom_headers' }
validates :custom_webhook_template, length: { maximum: 4096 }
- enum branch_filter_strategy: {
+ enum :branch_filter_strategy, {
wildcard: 0,
regex: 1,
all_branches: 2
- }, _prefix: true
+ }, prefix: true
- # rubocop: disable CodeReuse/ServiceClass
def execute(data, hook_name, idempotency_key: nil, force: false)
# hook.executable? is checked in WebHookService#execute
WebHookService.new(self, data, hook_name, idempotency_key: idempotency_key, force: force).execute
end
- # rubocop: enable CodeReuse/ServiceClass
- # rubocop: disable CodeReuse/ServiceClass
def async_execute(data, hook_name, idempotency_key: nil)
WebHookService.new(self, data, hook_name, idempotency_key: idempotency_key).async_execute if executable?
end
- # rubocop: enable CodeReuse/ServiceClass
# Allow urls pointing localhost and the local network
def allow_local_requests?
@@ -102,7 +106,7 @@
# Custom attributes to be included in the worker context.
def application_context
- { related_class: type }
+ { related_class: self.class.to_s }
end
# Exclude binary columns by default - they have no sensible JSON encoding
@@ -111,18 +115,15 @@
options[:except] = Array(options[:except]).dup
options[:except].concat [:encrypted_url_variables, :encrypted_url_variables_iv]
- super(options)
+ super
end
- # See app/validators/json_schemas/web_hooks_url_variables.json
- VARIABLE_REFERENCE_RE = /\{([A-Za-z]+[0-9]*(?:[._-][A-Za-z0-9]+)*)\}/
-
def interpolated_url(url = self.url, url_variables = self.url_variables)
return url unless url.include?('{')
vars = url_variables
- url.gsub(VARIABLE_REFERENCE_RE) do
- vars.fetch(_1.delete_prefix('{').delete_suffix('}'))
+ url.gsub(VARIABLE_REFERENCE_RE) do |match|
+ vars.fetch(match.delete_prefix('{').delete_suffix('}'))
end
rescue KeyError => e
raise InterpolationError, "Invalid URL template. Missing key #{e.key}"
@@ -132,6 +133,10 @@
token.present? ? SECRET_MASK : nil
end
+ def validate_public_url?
+ true
+ end
+
private
def reset_token
@@ -197,3 +202,5 @@
self.push_events_branch_filter = nil
end
end
+ end
+end$ diff -w --color --unified spec/models/hooks/system_hook_spec.rb spec/models/hooks/no_sti_system_hook_spec.rb--- spec/models/hooks/system_hook_spec.rb 2025-01-13 17:55:14.069594356 +0000
+++ spec/models/hooks/no_sti_system_hook_spec.rb 2025-01-13 17:55:14.170593044 +0000
@@ -2,10 +2,12 @@
require "spec_helper"
-RSpec.describe SystemHook, feature_category: :webhooks do
+RSpec.describe NoStiSystemHook, feature_category: :webhooks do
+ it_behaves_like 'a webhook', factory: :no_sti_system_hook, auto_disabling: false
+
it_behaves_like 'a hook that does not get automatically disabled on failure' do
- let(:hook) { build(:system_hook) }
- let(:hook_factory) { :system_hook }
+ let(:hook) { build(:no_sti_system_hook) }
+ let(:hook_factory) { :no_sti_system_hook }
let(:default_factory_arguments) { {} }
def find_hooks
@@ -13,8 +15,8 @@
end
end
- context 'default attributes' do
- let(:system_hook) { described_class.new }
+ describe 'default attributes' do
+ let(:no_sti_system_hook) { described_class.new }
it 'sets defined default parameters' do
attrs = {
@@ -22,8 +24,12 @@
repository_update_events: true,
merge_requests_events: false
}
- expect(system_hook).to have_attributes(attrs)
+ expect(no_sti_system_hook).to have_attributes(attrs)
+ end
end
+
+ describe 'associations' do
+ it { is_expected.not_to respond_to(:web_hook_logs) }
end
describe 'validations' do
@@ -41,190 +47,45 @@
end
end
- describe "execute", :sidekiq_might_not_need_inline do
- let_it_be(:system_hook) { create(:system_hook) }
- let_it_be(:user) { create(:user) }
- let(:project) { build(:project, namespace: user.namespace) }
- let(:group) { build(:group) }
- let(:params) do
- { name: 'John Doe', username: 'jduser', email: 'jg@example.com', password: User.random_password,
- organization_id: group.organization_id }
- end
-
- before do
- WebMock.stub_request(:post, system_hook.url)
- end
-
- it "project_create hook" do
- Projects::CreateService.new(user, name: 'empty').execute
- expect(WebMock).to have_requested(:post, system_hook.url).with(
- body: /project_create/,
- headers: { 'Content-Type' => 'application/json', 'X-Gitlab-Event' => 'System Hook' }
- ).once
- end
-
- it "project_destroy hook" do
- Projects::DestroyService.new(project, user, {}).async_execute
-
- expect(WebMock).to have_requested(:post, system_hook.url).with(
- body: /project_destroy/,
- headers: { 'Content-Type' => 'application/json', 'X-Gitlab-Event' => 'System Hook' }
- ).once
- end
-
- it "user_create hook" do
- Users::CreateService.new(nil, params).execute
-
- expect(WebMock).to have_requested(:post, system_hook.url).with(
- body: /user_create/,
- headers: { 'Content-Type' => 'application/json', 'X-Gitlab-Event' => 'System Hook' }
- ).once
- end
-
- it "user_destroy hook" do
- user.destroy!
-
- expect(WebMock).to have_requested(:post, system_hook.url).with(
- body: /user_destroy/,
- headers: { 'Content-Type' => 'application/json', 'X-Gitlab-Event' => 'System Hook' }
- ).once
- end
-
- it "project member create hook" do
- project.add_maintainer(user)
-
- expect(WebMock).to have_requested(:post, system_hook.url).with(
- body: /user_add_to_team/,
- headers: { 'Content-Type' => 'application/json', 'X-Gitlab-Event' => 'System Hook' }
- ).once
- end
-
- it "project member destroy hook" do
- project.add_maintainer(user)
- project.project_members.destroy_all # rubocop: disable Cop/DestroyAll
-
- expect(WebMock).to have_requested(:post, system_hook.url).with(
- body: /user_remove_from_team/,
- headers: { 'Content-Type' => 'application/json', 'X-Gitlab-Event' => 'System Hook' }
- ).once
- end
-
- it "project member update hook" do
- project.add_guest(user)
-
- expect(WebMock).to have_requested(:post, system_hook.url).with(
- body: /user_update_for_team/,
- headers: { 'Content-Type' => 'application/json', 'X-Gitlab-Event' => 'System Hook' }
- ).once
- end
-
- %i[project group].each do |parent|
- it "#{parent} member access request hook" do
- create(:"#{parent}_member", requested_at: Time.current.utc)
-
- expect(WebMock).to have_requested(:post, system_hook.url).with(
- body: /user_access_request_to_#{parent}/,
- headers: { 'Content-Type' => 'application/json', 'X-Gitlab-Event' => 'System Hook' }
- ).once
- end
-
- it "#{parent} member access request revoked hook" do
- member = create(:"#{parent}_member", requested_at: Time.current.utc)
- member.destroy!
-
- expect(WebMock).to have_requested(:post, system_hook.url).with(
- body: /user_access_request_revoked_for_#{parent}/,
- headers: { 'Content-Type' => 'application/json', 'X-Gitlab-Event' => 'System Hook' }
- ).once
- end
- end
-
- it 'group create hook' do
- create(:group)
-
- expect(WebMock).to have_requested(:post, system_hook.url).with(
- body: /group_create/,
- headers: { 'Content-Type' => 'application/json', 'X-Gitlab-Event' => 'System Hook' }
- ).once
- end
-
- it 'group destroy hook' do
- create(:group).destroy!
-
- expect(WebMock).to have_requested(:post, system_hook.url).with(
- body: /group_destroy/,
- headers: { 'Content-Type' => 'application/json', 'X-Gitlab-Event' => 'System Hook' }
- ).once
- end
-
- it 'group member create hook' do
- group.add_maintainer(user)
-
- expect(WebMock).to have_requested(:post, system_hook.url).with(
- body: /user_add_to_group/,
- headers: { 'Content-Type' => 'application/json', 'X-Gitlab-Event' => 'System Hook' }
- ).once
- end
-
- it 'group member destroy hook' do
- group.add_maintainer(user)
- group.group_members.destroy_all # rubocop: disable Cop/DestroyAll
-
- expect(WebMock).to have_requested(:post, system_hook.url).with(
- body: /user_remove_from_group/,
- headers: { 'Content-Type' => 'application/json', 'X-Gitlab-Event' => 'System Hook' }
- ).once
- end
-
- it 'group member update hook' do
- group = create(:group)
- group.add_guest(user)
- group.add_maintainer(user)
-
- expect(WebMock).to have_requested(:post, system_hook.url).with(
- body: /user_update_for_group/,
- headers: { 'Content-Type' => 'application/json', 'X-Gitlab-Event' => 'System Hook' }
- ).once
- end
- end
-
describe '.repository_update_hooks' do
it 'returns hooks for repository update events only' do
- hook = create(:system_hook, repository_update_events: true)
- create(:system_hook, repository_update_events: false)
+ hook = create(:no_sti_system_hook, repository_update_events: true)
+ create(:no_sti_system_hook, repository_update_events: false)
expect(described_class.repository_update_hooks).to eq([hook])
end
end
describe 'execute WebHookService' do
- let(:hook) { build(:system_hook) }
+ let(:hook) { build(:no_sti_system_hook) }
let(:data) { { key: 'value' } }
- let(:hook_name) { 'system_hook' }
+ let(:hook_name) { 'no_sti_system_hook' }
+ let(:web_hook_service) { instance_double(WebHookService, execute: true) }
it '#execute' do
- expect(WebHookService).to receive(:new).with(hook, data, hook_name, idempotency_key: anything,
- force: false).and_call_original
+ expect(WebHookService).to receive(:new).with(hook, data, hook_name, idempotency_key: anything, force: false)
+ .and_return(web_hook_service)
- expect_any_instance_of(WebHookService).to receive(:execute)
+ expect(web_hook_service).to receive(:execute)
hook.execute(data, hook_name)
end
it '#async_execute' do
- expect(WebHookService).to receive(:new).with(hook, data, hook_name, idempotency_key: anything).and_call_original
+ expect(WebHookService).to receive(:new).with(hook, data, hook_name, idempotency_key: anything)
+ .and_return(web_hook_service)
- expect_any_instance_of(WebHookService).to receive(:async_execute)
+ expect(web_hook_service).to receive(:async_execute)
hook.async_execute(data, hook_name)
end
end
describe '#application_context' do
- let(:hook) { build(:system_hook) }
+ let(:hook) { build(:no_sti_system_hook) }
it 'includes the type' do
expect(hook.application_context).to eq(
- related_class: 'SystemHook'
+ related_class: 'NoStiSystemHook'
)
end
end$ diff -w --color --unified spec/models/hooks/web_hook_spec.rb spec/support/shared_examples/models/concerns/web_hooks/web_hook_shared_examples.rb--- spec/models/hooks/web_hook_spec.rb 2025-01-13 17:55:14.069594356 +0000
+++ spec/support/shared_examples/models/concerns/web_hooks/web_hook_shared_examples.rb 2025-01-13 17:55:14.170593044 +0000
@@ -2,12 +2,10 @@
require 'spec_helper'
-RSpec.describe WebHook, feature_category: :webhooks do
+RSpec.shared_examples 'a webhook' do |factory:, auto_disabling: true|
include AfterNextHelpers
- let_it_be(:project) { create(:project) }
-
- let(:hook) { build(:project_hook, project: project) }
+ let(:hook) { build(factory) }
around do |example|
if example.metadata[:skip_freeze_time]
@@ -17,10 +15,6 @@
end
end
- describe 'associations' do
- it { is_expected.to have_many(:web_hook_logs) }
- end
-
describe 'validations' do
it { is_expected.to validate_presence_of(:url) }
it { is_expected.to validate_length_of(:custom_webhook_template).is_at_most(4096) }
@@ -34,7 +28,7 @@
it { is_expected.to allow_value({ 'x' => 'y' }).for(:url_variables) }
it { is_expected.to allow_value({ 'x' => ('a' * 2048) }).for(:url_variables) }
it { is_expected.to allow_value({ 'foo' => 'bar', 'bar' => 'baz' }).for(:url_variables) }
- it { is_expected.to allow_value((1..20).to_h { ["k#{_1}", 'value'] }).for(:url_variables) }
+ it { is_expected.to allow_value((1..20).to_h { |i| ["k#{i}", 'value'] }).for(:url_variables) }
it { is_expected.to allow_value({ 'MY-TOKEN' => 'bar' }).for(:url_variables) }
it { is_expected.to allow_value({ 'my_secr3t-token' => 'bar' }).for(:url_variables) }
it { is_expected.to allow_value({ 'x-y-z' => 'bar' }).for(:url_variables) }
@@ -50,7 +44,7 @@
it { is_expected.not_to allow_value({ 'has spaces' => 'foo' }).for(:url_variables) }
it { is_expected.not_to allow_value({ '' => 'foo' }).for(:url_variables) }
it { is_expected.not_to allow_value({ '1foo' => 'foo' }).for(:url_variables) }
- it { is_expected.not_to allow_value((1..21).to_h { ["k#{_1}", 'value'] }).for(:url_variables) }
+ it { is_expected.not_to allow_value((1..21).to_h { |i| ["k#{i}", 'value'] }).for(:url_variables) }
it { is_expected.not_to allow_value({ 'MY--TOKEN' => 'foo' }).for(:url_variables) }
it { is_expected.not_to allow_value({ 'MY__SECRET' => 'foo' }).for(:url_variables) }
it { is_expected.not_to allow_value({ 'x-_y' => 'foo' }).for(:url_variables) }
@@ -66,7 +60,7 @@
it { is_expected.to allow_value({ 'x' => 'y' }).for(:custom_headers) }
it { is_expected.to allow_value({ 'x' => ('a' * 2048) }).for(:custom_headers) }
it { is_expected.to allow_value({ 'foo' => 'bar', 'bar' => 'baz' }).for(:custom_headers) }
- it { is_expected.to allow_value((1..20).to_h { ["k#{_1}", 'value'] }).for(:custom_headers) }
+ it { is_expected.to allow_value((1..20).to_h { |i| ["k#{i}", 'value'] }).for(:custom_headers) }
it { is_expected.to allow_value({ 'MY-TOKEN' => 'bar' }).for(:custom_headers) }
it { is_expected.to allow_value({ 'my_secr3t-token' => 'bar' }).for(:custom_headers) }
it { is_expected.to allow_value({ 'x-y-z' => 'bar' }).for(:custom_headers) }
@@ -82,7 +76,7 @@
it { is_expected.not_to allow_value({ 'has spaces' => 'foo' }).for(:custom_headers) }
it { is_expected.not_to allow_value({ '' => 'foo' }).for(:custom_headers) }
it { is_expected.not_to allow_value({ '1foo' => 'foo' }).for(:custom_headers) }
- it { is_expected.not_to allow_value((1..21).to_h { ["k#{_1}", 'value'] }).for(:custom_headers) }
+ it { is_expected.not_to allow_value((1..21).to_h { |i| ["k#{i}", 'value'] }).for(:custom_headers) }
it { is_expected.not_to allow_value({ 'MY--TOKEN' => 'foo' }).for(:custom_headers) }
it { is_expected.not_to allow_value({ 'MY__SECRET' => 'foo' }).for(:custom_headers) }
it { is_expected.not_to allow_value({ 'x-_y' => 'foo' }).for(:custom_headers) }
@@ -163,39 +157,21 @@
context 'with "all branches" strategy' do
let(:strategy) { 'all_branches' }
+ let(:allowed_values) do
+ ["good_branch_name", "another/good-branch_name", "good branch name", "good~branchname", "good_branchname(",
+ "good_branchname[", ""]
+ end
- it {
- is_expected.to allow_values(
- "good_branch_name",
- "another/good-branch_name",
- "good branch name",
- "good~branchname",
- "good_branchname(",
- "good_branchname[",
- ""
- ).for(:push_events_branch_filter)
- }
+ it { is_expected.to allow_values(*allowed_values).for(:push_events_branch_filter) }
end
context 'with "wildcard" strategy' do
let(:strategy) { 'wildcard' }
+ let(:allowed_values) { ["good_branch_name", "another/good-branch_name", "good_branch_name(", ""] }
+ let(:disallowed_values) { ["bad branch name", "bad~branchname", "bad_branch_name["] }
- it {
- is_expected.to allow_values(
- "good_branch_name",
- "another/good-branch_name",
- "good_branch_name(",
- ""
- ).for(:push_events_branch_filter)
- }
-
- it {
- is_expected.not_to allow_values(
- "bad branch name",
- "bad~branchname",
- "bad_branch_name["
- ).for(:push_events_branch_filter)
- }
+ it { is_expected.to allow_values(*allowed_values).for(:push_events_branch_filter) }
+ it { is_expected.not_to allow_values(*disallowed_values).for(:push_events_branch_filter) }
it 'gets rid of whitespace' do
hook.push_events_branch_filter = ' branch '
@@ -213,23 +189,17 @@
context 'with "regex" strategy' do
let(:strategy) { 'regex' }
+ let(:allowed_values) do
+ ["good_branch_name", "another/good-branch_name", "good branch name", "good~branch~name", ""]
+ end
- it {
- is_expected.to allow_values(
- "good_branch_name",
- "another/good-branch_name",
- "good branch name",
- "good~branch~name",
- ""
- ).for(:push_events_branch_filter)
- }
-
+ it { is_expected.to allow_values(*allowed_values).for(:push_events_branch_filter) }
it { is_expected.not_to allow_values("bad_branch_name(", "bad_branch_name[").for(:push_events_branch_filter) }
end
end
describe 'before_validation :reset_token' do
- subject(:hook) { build_stubbed(:project_hook, :token, project: project) }
+ subject(:hook) { build_stubbed(factory, :token) }
it 'resets token if url changed' do
hook.url = 'https://webhook.example.com/new-hook'
@@ -259,7 +229,7 @@
end
describe 'before_validation :reset_url_variables' do
- subject(:hook) { build_stubbed(:project_hook, :url_variables, project: project, url: 'http://example.com/{abc}') }
+ subject(:hook) { build_stubbed(factory, :url_variables, url: 'http://example.com/{abc}') }
it 'resets url variables if url changed' do
hook.url = 'http://example.com/new-hook'
@@ -307,7 +277,7 @@
end
context 'without url variables' do
- subject(:hook) { build_stubbed(:project_hook, project: project, url: 'http://example.com', url_variables: nil) }
+ subject(:hook) { build_stubbed(factory, url: 'http://example.com', url_variables: nil) }
it 'does not reset url variables' do
hook.url = 'http://example.com/{one}/{two}'
@@ -320,7 +290,7 @@
end
describe 'before_validation :reset_custom_headers' do
- subject(:hook) { build_stubbed(:project_hook, :url_variables, project: project, url: 'http://example.com/{abc}', custom_headers: { test: 'blub' }) }
+ subject(:hook) { build_stubbed(factory, :url_variables, url: 'http://example.com/{abc}', custom_headers: { test: 'blub' }) }
it 'resets custom headers if url changed' do
hook.url = 'http://example.com/new-hook'
@@ -355,7 +325,7 @@
it "only consider these branch filter strategies are valid" do
expected_valid_types = %w[all_branches regex wildcard]
- expect(described_class.branch_filter_strategies.keys).to contain_exactly(*expected_valid_types)
+ expect(described_class.branch_filter_strategies.keys).to match_array(expected_valid_types)
end
end
@@ -367,7 +337,7 @@
describe 'execute' do
let(:data) { { key: 'value' } }
- let(:hook_name) { 'project hook' }
+ let(:hook_name) { 'the hook name' }
it '#execute' do
expect_next(WebHookService).to receive(:execute)
@@ -378,7 +348,7 @@
it 'passes force: false to the web hook service by default' do
expect(WebHookService)
.to receive(:new).with(hook, data, hook_name, idempotency_key: anything,
- force: false).and_return(double(execute: :done))
+ force: false).and_return(instance_double(WebHookService, execute: :done))
expect(hook.execute(data, hook_name)).to eq :done
end
@@ -386,7 +356,7 @@
it 'passes force: true to the web hook service if required' do
expect(WebHookService)
.to receive(:new).with(hook, data, hook_name, idempotency_key: anything,
- force: true).and_return(double(execute: :forced))
+ force: true).and_return(instance_double(WebHookService, execute: :forced))
expect(hook.execute(data, hook_name, force: true)).to eq :forced
end
@@ -397,7 +367,7 @@
expect(WebHookService)
.to receive(:new)
.with(anything, anything, anything, idempotency_key: idempotency_key, force: anything)
- .and_return(double(execute: :done))
+ .and_return(instance_double(WebHookService, execute: :done))
expect(hook.execute(data, hook_name, idempotency_key: idempotency_key)).to eq :done
end
@@ -405,7 +375,7 @@
it 'forwards a nil idempotency key to the WebHook service when not supplied' do
expect(WebHookService)
.to receive(:new).with(anything, anything, anything, idempotency_key: nil,
- force: anything).and_return(double(execute: :done))
+ force: anything).and_return(instance_double(WebHookService, execute: :done))
expect(hook.execute(data, hook_name)).to eq :done
end
@@ -413,7 +383,7 @@
describe 'async_execute' do
let(:data) { { key: 'value' } }
- let(:hook_name) { 'project hook' }
+ let(:hook_name) { 'the hook name' }
it '#async_execute' do
expect_next(WebHookService).to receive(:async_execute)
@@ -427,7 +397,7 @@
expect(WebHookService)
.to receive(:new)
.with(anything, anything, anything, idempotency_key: idempotency_key)
- .and_return(double(async_execute: :done))
+ .and_return(instance_double(WebHookService, async_execute: :done))
expect(hook.async_execute(data, hook_name, idempotency_key: idempotency_key)).to eq :done
end
@@ -435,7 +405,7 @@
it 'forwards a nil idempotency key to the WebHook service when not supplied' do
expect(WebHookService)
.to receive(:new).with(anything, anything, anything,
- idempotency_key: nil).and_return(double(async_execute: :done))
+ idempotency_key: nil).and_return(instance_double(WebHookService, async_execute: :done))
expect(hook.async_execute(data, hook_name)).to eq :done
end
@@ -449,15 +419,6 @@
end
end
- describe '#destroy' do
- it 'does not cascade to web_hook_logs' do
- web_hook = create(:project_hook)
- create_list(:web_hook_log, 3, web_hook: web_hook)
-
- expect { web_hook.destroy! }.not_to change(web_hook.web_hook_logs, :count)
- end
- end
-
describe '#next_backoff' do
before do
hook.backoff_count = backoff_count
@@ -539,7 +500,7 @@
end
describe '#interpolated_url' do
- subject(:hook) { build(:project_hook, project: project) }
+ subject(:hook) { build(factory) }
context 'when the hook URL does not contain variables' do
before do
@@ -592,16 +553,16 @@
it { expect(hook.masked_token).to be_nil }
context 'with a token' do
- let(:hook) { build(:project_hook, :token, project: project) }
+ let(:hook) { build(factory, :token) }
it { expect(hook.masked_token).to eq described_class::SECRET_MASK }
end
end
- describe '#backoff!' do
+ describe '#backoff!', if: auto_disabling do
context 'when we have not backed off before' do
it 'increments the recent_failures count but does not disable the hook yet' do
- expect { hook.backoff! }.to change(hook, :recent_failures).to(1)
+ expect { hook.backoff! }.to change { hook.recent_failures }.to(1)
expect(hook.class.executable).to include(hook)
end
end
@@ -697,9 +658,9 @@
end
end
- describe '#failed!' do
+ describe '#failed!', if: auto_disabling do
it 'increments the recent_failures count but does not disable the hook yet' do
- expect { hook.failed! }.to change(hook, :recent_failures).to(1)
+ expect { hook.failed! }.to change { hook.recent_failures }.to(1)
expect(hook.class.executable).to include(hook)
end
References
Please include cross links to any resources that are relevant to this MR. This will give reviewers and future readers helpful context to give an efficient review of the changes introduced.
MR acceptance checklist
Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.
Screenshots or screen recordings
Screenshots are required for UI changes, and strongly recommended for all other merge requests.
| Before | After |
|---|---|
How to set up and validate locally
There should be no changes to the current behaviour.
- Run a local server that will just echo any requests it makes, such as
http-echo-server
npm install http-echo-server
PORT=8080 npx http-echo-server- Create a system hook from http://172.16.123.1:3000/admin/hooks
- Test the hook, by performing the appropriate action
- Confirm the request is made in the
http-echo-serverterminal - Delete the hook
- Create a project hook by going to a projects settings -> Webhooks and test it in the same way
Edited by Keeyan Nejad