Read about our upcoming Code of Conduct on this issue

Commit 476da708 authored by Duana Saskia's avatar Duana Saskia
Browse files

Refactor: move active hook filter to TriggerableHooks

parent b3d22498a5c7
......@@ -28,6 +28,12 @@ def hooks_for(trigger)
public_send(trigger) # rubocop:disable GitlabSecurity/PublicSend
end
def select_active(hooks_scope, data)
select do |hook|
ActiveHookFilter.new(hook).matches?(hooks_scope, data)
end
end
private
def triggerable_hooks(hooks)
......
......@@ -1163,7 +1163,7 @@ def owner
def execute_hooks(data, hooks_scope = :push_hooks)
run_after_commit_or_now do
hooks.hooks_for(hooks_scope).select {|hook| ActiveHookFilter.new(hook).matches?(hooks_scope, data)}.each do |hook|
hooks.hooks_for(hooks_scope).select_active(hooks_scope, data).each do |hook|
hook.async_execute(data, hooks_scope.to_s)
end
SystemHooksService.new.execute_hooks(data, hooks_scope)
......
......@@ -16,6 +16,7 @@ def validate_each(record, attribute, value)
if value.present?
value_without_wildcards = value.tr('*', 'x')
unless Gitlab::GitRefValidator.validate(value_without_wildcards)
record.errors[attribute] << "is not a valid branch name"
end
......
......@@ -40,4 +40,28 @@ class TestableHook < WebHook
end
end
end
describe '.select_active' do
it 'returns hooks that match the active filter' do
TestableHook.create!(url: 'http://example1.com', push_events: true)
TestableHook.create!(url: 'http://example2.com', push_events: true)
filter1 = double(:filter1)
filter2 = double(:filter2)
allow(ActiveHookFilter).to receive(:new).exactly(2).times.and_return(filter1, filter2)
expect(filter1).to receive(:matches?).and_return(true)
expect(filter2).to receive(:matches?).and_return(false)
hooks = TestableHook.push_hooks.order_id_asc
expect(hooks.select_active(:push_hooks, {})).to eq [hooks.first]
end
it 'returns empty list if no hooks match the active filter' do
TestableHook.create!(url: 'http://example1.com', push_events: true)
filter = double(:filter)
allow(ActiveHookFilter).to receive(:new).and_return(filter)
expect(filter).to receive(:matches?).and_return(false)
expect(TestableHook.push_hooks.select_active(:push_hooks, {})).to eq []
end
end
end
......@@ -11,28 +11,29 @@
context 'branch filter is specified' do
let(:branch_filter) { 'master' }
it 'returns true if branch matches' do
expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to eq(true)
expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to be true
end
it 'returns false if branch does not match' do
expect(filter.matches?(:push_hooks, { ref: 'refs/heads/my_branch' })).to eq(false)
expect(filter.matches?(:push_hooks, { ref: 'refs/heads/my_branch' })).to be false
end
it 'returns false if ref is nil' do
expect(filter.matches?(:push_hooks, {})).to eq(false)
expect(filter.matches?(:push_hooks, {})).to be false
end
context 'branch filter contains wildcard' do
let(:branch_filter) { 'features/*' }
it 'returns true if branch matches' do
expect(filter.matches?(:push_hooks, { ref: 'refs/heads/features/my-branch' })).to eq(true)
expect(filter.matches?(:push_hooks, { ref: 'refs/heads/features/my-branch/something' })).to eq(true)
expect(filter.matches?(:push_hooks, { ref: 'refs/heads/features/my-branch' })).to be true
expect(filter.matches?(:push_hooks, { ref: 'refs/heads/features/my-branch/something' })).to be true
end
it 'returns false if branch does not match' do
expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to eq(false)
expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to be false
end
end
end
......@@ -41,7 +42,7 @@
let(:branch_filter) { nil }
it 'returns true' do
expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to eq(true)
expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to be true
end
end
......@@ -49,7 +50,7 @@
let(:branch_filter) { '' }
it 'acts like branch is not specified' do
expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to eq(true)
expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to be true
end
end
end
......@@ -60,7 +61,7 @@
end
it 'returns true as branch filters are not yet supported for these' do
expect(filter.matches?(:issues_events, { ref: 'refs/heads/master' })).to eq(true)
expect(filter.matches?(:issues_events, { ref: 'refs/heads/master' })).to be true
end
end
end
......
......@@ -3672,36 +3672,36 @@ def domain_variable
describe '#execute_hooks' do
let(:data) { { ref: 'refs/heads/master', data: 'data' } }
it 'executes active projects hooks with the specified scope' do
expect_any_instance_of(ActiveHookFilter).to receive(:matches?)
.with(:tag_push_hooks, data)
.and_return(true)
hook = create(:project_hook, merge_requests_events: false, tag_push_events: true)
hook = create(:project_hook, merge_requests_events: false, push_events: true)
expect(ProjectHook).to receive(:select_active)
.with(:push_hooks, data)
.and_return([hook])
project = create(:project, hooks: [hook])
expect_any_instance_of(ProjectHook).to receive(:async_execute).once
project.execute_hooks(data, :tag_push_hooks)
project.execute_hooks(data, :push_hooks)
end
it 'does not execute project hooks that dont match the specified scope' do
hook = create(:project_hook, merge_requests_events: true, tag_push_events: false)
hook = create(:project_hook, merge_requests_events: true, push_events: false)
project = create(:project, hooks: [hook])
expect_any_instance_of(ProjectHook).not_to receive(:async_execute).once
project.execute_hooks(data, :tag_push_hooks)
project.execute_hooks(data, :push_hooks)
end
it 'does not execute project hooks which are not active' do
expect_any_instance_of(ActiveHookFilter).to receive(:matches?)
.with(:tag_push_hooks, data)
.and_return(false)
hook = create(:project_hook, tag_push_events: true)
hook = create(:project_hook, push_events: true)
expect(ProjectHook).to receive(:select_active)
.with(:push_hooks, data)
.and_return([])
project = create(:project, hooks: [hook])
expect_any_instance_of(ProjectHook).not_to receive(:async_execute).once
project.execute_hooks(data, :tag_push_hooks)
project.execute_hooks(data, :push_hooks)
end
it 'executes the system hooks with the specified scope' do
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment